From 3bb29dd707fbc825501c30ed9a6fe4aecdf4fd22 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Nov 2017 11:02:40 -0500 Subject: [PATCH 1/2] 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 2/2] 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;