From cabcb752d7ecc2d16e6cb630b3de0684b4f97ec5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 10:10:13 -0500 Subject: [PATCH 1/5] 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/5] 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 3bb29dd707fbc825501c30ed9a6fe4aecdf4fd22 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 11:02:40 -0500 Subject: [PATCH 3/5] Correctly handle partial success in consensus diff calculation. Previously, if store_multiple() reported a partial success, we would store all the handles it gave us as if they had succeeded. But it's possible for the diff to be only partially successful -- for example, if LZMA failed but the other compressors succeeded. Fixes bug 24086; bugfix on 0.3.1.1-alpha. --- changes/bug24086 | 7 +++++++ src/or/consdiffmgr.c | 9 +++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 changes/bug24086 diff --git a/changes/bug24086 b/changes/bug24086 new file mode 100644 index 0000000000..2ae0b37e65 --- /dev/null +++ b/changes/bug24086 @@ -0,0 +1,7 @@ + o Minor bugfixes (directory cache): + - When a consensus diff calculation is only partially successful, only + record the successful parts as having succeeded. Partial success + can happen if (for example) one compression method fails but + the others succeed. Previously we misrecorded all the calculations as + having succeeded, which would later cause a nonfatal assertion failure. + Fixes bug 24086; bugfix on 0.3.1.1-alpha. diff --git a/src/or/consdiffmgr.c b/src/or/consdiffmgr.c index 831d5d45c3..8c25ff201c 100644 --- a/src/or/consdiffmgr.c +++ b/src/or/consdiffmgr.c @@ -1589,8 +1589,13 @@ consensus_diff_worker_replyfn(void *work_) for (u = 0; u < ARRAY_LENGTH(handles); ++u) { compress_method_t method = compress_diffs_with[u]; if (cache) { - cdm_diff_ht_set_status(flav, from_sha3, to_sha3, method, status, - handles[u]); + consensus_cache_entry_handle_t *h = handles[u]; + int this_status = status; + if (h == NULL) { + this_status = CDM_DIFF_ERROR; + } + tor_assert_nonfatal(h != NULL || this_status == CDM_DIFF_ERROR); + cdm_diff_ht_set_status(flav, from_sha3, to_sha3, method, this_status, h); } else { consensus_cache_entry_handle_free(handles[u]); } From 68c21860e32ca04d77c2bfbf7576b96de5110f59 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 11:04:44 -0500 Subject: [PATCH 4/5] Add another assertion to check for 24086 root causes In cdm_diff_ht_set_status(), we shouldn't have been allowing the status CDM_DIFF_PRESENT to be set if there wasn't actually a handle. --- src/or/consdiffmgr.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/consdiffmgr.c b/src/or/consdiffmgr.c index 8c25ff201c..c5f55b6f3f 100644 --- a/src/or/consdiffmgr.c +++ b/src/or/consdiffmgr.c @@ -283,6 +283,10 @@ cdm_diff_ht_set_status(consensus_flavor_t flav, int status, consensus_cache_entry_handle_t *handle) { + if (handle == NULL) { + tor_assert_nonfatal(status != CDM_DIFF_PRESENT); + } + struct cdm_diff_t search, *ent; memset(&search, 0, sizeof(cdm_diff_t)); search.flavor = flav; From ea13a47791c1a6b50fed27322a073c54868ac738 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 21 Nov 2017 12:36:32 -0500 Subject: [PATCH 5/5] 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",