From cabcb752d7ecc2d16e6cb630b3de0684b4f97ec5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 10:10:13 -0500 Subject: [PATCH 1/3] In storagedir, take more care with errno on empty or mislabeled file Required for 24099 fix -- we won't be able to act based on errno unless we can trust it. --- src/common/storagedir.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index 31933f64c2..c471ea911f 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -187,14 +187,19 @@ storage_dir_get_usage(storage_dir_t *d) return d->usage; } -/** Mmap a specified file within d. */ +/** Mmap a specified file within d. + * + * On failure, return NULL and set errno as for tor_mmap_file(). */ tor_mmap_t * storage_dir_map(storage_dir_t *d, const char *fname) { char *path = NULL; tor_asprintf(&path, "%s/%s", d->directory, fname); tor_mmap_t *result = tor_mmap_file(path); + int errval = errno; tor_free(path); + if (result == NULL) + errno = errval; return result; } @@ -364,6 +369,10 @@ storage_dir_save_labeled_to_file(storage_dir_t *d, * the data's size into *sz_out. On success, also return a tor_mmap_t * object whose contents should not be used -- it needs to be kept around, * though, for as long as data_out is going to be valid. + * + * On failure, set errno as for tor_mmap_file() if the file was missing or + * empty, and set errno to EINVAL if the file was not in the labeled + * format expected. */ tor_mmap_t * storage_dir_map_labeled(storage_dir_t *dir, @@ -373,13 +382,20 @@ storage_dir_map_labeled(storage_dir_t *dir, size_t *sz_out) { tor_mmap_t *m = storage_dir_map(dir, fname); - if (! m) + int errval; + if (! m) { + errval = errno; goto err; + } const char *nulp = memchr(m->data, '\0', m->size); - if (! nulp) + if (! nulp) { + errval = EINVAL; goto err; - if (labels_out && config_get_lines(m->data, labels_out, 0) < 0) + } + if (labels_out && config_get_lines(m->data, labels_out, 0) < 0) { + errval = EINVAL; goto err; + } size_t offset = nulp - m->data + 1; tor_assert(offset <= m->size); *data_out = (const uint8_t *)(m->data + offset); @@ -388,6 +404,7 @@ storage_dir_map_labeled(storage_dir_t *dir, return m; err: tor_munmap_file(m); + errno = errval; return NULL; } From c8ee12b2e8108658d647aedb92885311291b6f71 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 10:14:41 -0500 Subject: [PATCH 2/3] Recover better from empty/invalid storagedir files If we can't read a file because of an FS issue, we say "we can't read that" and move on. But if we can't read it because it's empty, because it has no labels, or because its labels are misformatted, we should remove it. Fixes bug 24099; bugfix on 0.3.1.1-alpha. --- changes/bug24099 | 4 ++++ src/or/conscache.c | 13 ++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 changes/bug24099 diff --git a/changes/bug24099 b/changes/bug24099 new file mode 100644 index 0000000000..dca3992664 --- /dev/null +++ b/changes/bug24099 @@ -0,0 +1,4 @@ + o Minor bugfixes (directory cache): + - Recover better from empty or corrupt files in the consensus cache + directory. Fixes bug 24099; bugfix on 0.3.1.1-alpha. + diff --git a/src/or/conscache.c b/src/or/conscache.c index 9e13ce8e43..4919dba3d2 100644 --- a/src/or/conscache.c +++ b/src/or/conscache.c @@ -539,9 +539,16 @@ consensus_cache_rescan(consensus_cache_t *cache) map = storage_dir_map_labeled(cache->dir, fname, &labels, &body, &bodylen); if (! map) { - /* Can't load this; continue */ - log_warn(LD_FS, "Unable to map file %s from consensus cache: %s", - escaped(fname), strerror(errno)); + if (errno == ERANGE || errno == EINVAL) { + log_warn(LD_FS, "Found %s file %s in consensus cache; removing it.", + errno == ERANGE ? "empty" : "misformatted", + escaped(fname)); + storage_dir_remove_file(cache->dir, fname); + } else { + /* Can't load this; continue */ + log_warn(LD_FS, "Unable to map file %s from consensus cache: %s", + escaped(fname), strerror(errno)); + } continue; } consensus_cache_entry_t *ent = From ea13a47791c1a6b50fed27322a073c54868ac738 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 21 Nov 2017 12:36:32 -0500 Subject: [PATCH 3/3] add an explanatory comment about the error codes --- src/or/conscache.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/conscache.c b/src/or/conscache.c index 4919dba3d2..33a5495974 100644 --- a/src/or/conscache.c +++ b/src/or/conscache.c @@ -539,6 +539,10 @@ consensus_cache_rescan(consensus_cache_t *cache) map = storage_dir_map_labeled(cache->dir, fname, &labels, &body, &bodylen); if (! map) { + /* The ERANGE error might come from tor_mmap_file() -- it means the file + * was empty. EINVAL might come from ..map_labeled() -- it means the + * file was misformatted. In both cases, we should just delete it. + */ if (errno == ERANGE || errno == EINVAL) { log_warn(LD_FS, "Found %s file %s in consensus cache; removing it.", errno == ERANGE ? "empty" : "misformatted",