commit e014b72b73b2a299068f1ca5b7a22f2bea2f58f8 Author: Nick Mathewson nickm@torproject.org Date: Tue Sep 11 10:09:12 2018 -0400
Stop memcpy'ing uncompressed consensuses when making diffs --- src/feature/dircache/consdiffmgr.c | 50 +++++++++++++++++++++++--------------- src/feature/dircache/consdiffmgr.h | 5 ++-- src/test/fuzz/fuzz_diff.c | 17 ++++++++++--- src/test/test_consdiffmgr.c | 16 ++++++------ 4 files changed, 56 insertions(+), 32 deletions(-)
diff --git a/src/feature/dircache/consdiffmgr.c b/src/feature/dircache/consdiffmgr.c index 7999df08d..bf3a0ef3c 100644 --- a/src/feature/dircache/consdiffmgr.c +++ b/src/feature/dircache/consdiffmgr.c @@ -1387,19 +1387,21 @@ typedef struct consensus_diff_worker_job_t { } consensus_diff_worker_job_t;
/** Given a consensus_cache_entry_t, check whether it has a label claiming - * that it was compressed. If so, uncompress its contents into <b>out</b> and - * set <b>outlen</b> to hold their size. If not, just copy the body into - * <b>out</b> and set <b>outlen</b> to its length. Return 0 on success, - * -1 on failure. - * - * In all cases, the output is nul-terminated. */ + * that it was compressed. If so, uncompress its contents into *<b>out</b> and + * set <b>outlen</b> to hold their size, and set *<b>owned_out</b> to a pointer + * that the caller will need to free. If not, just set *<b>out</b> and + * <b>outlen</b> to its extent in memory. Return 0 on success, -1 on failure. + **/ STATIC int -uncompress_or_copy(char **out, size_t *outlen, - consensus_cache_entry_t *ent) +uncompress_or_set_ptr(const char **out, size_t *outlen, + char **owned_out, + consensus_cache_entry_t *ent) { const uint8_t *body; size_t bodylen;
+ *owned_out = NULL; + if (consensus_cache_entry_get_body(ent, &body, &bodylen) < 0) return -1;
@@ -1410,8 +1412,17 @@ uncompress_or_copy(char **out, size_t *outlen, if (lv_compression) method = compression_method_get_by_name(lv_compression);
- return tor_uncompress(out, outlen, (const char *)body, bodylen, + int rv; + if (method == NO_METHOD) { + *out = (const char *)body; + *outlen = bodylen; + rv = 0; + } else { + rv = tor_uncompress(owned_out, outlen, (const char *)body, bodylen, method, 1, LOG_WARN); + *out = *owned_out; + } + return rv; }
/** @@ -1478,16 +1489,17 @@ consensus_diff_worker_threadfn(void *state_, void *work_)
char *consensus_diff; { - char *diff_from_nt = NULL, *diff_to_nt = NULL; + const char *diff_from_nt = NULL, *diff_to_nt = NULL; + char *owned1 = NULL, *owned2 = NULL; size_t diff_from_nt_len, diff_to_nt_len;
- if (uncompress_or_copy(&diff_from_nt, &diff_from_nt_len, - job->diff_from) < 0) { + if (uncompress_or_set_ptr(&diff_from_nt, &diff_from_nt_len, &owned1, + job->diff_from) < 0) { return WQ_RPL_REPLY; } - if (uncompress_or_copy(&diff_to_nt, &diff_to_nt_len, - job->diff_to) < 0) { - tor_free(diff_from_nt); + if (uncompress_or_set_ptr(&diff_to_nt, &diff_to_nt_len, &owned2, + job->diff_to) < 0) { + tor_free(owned1); return WQ_RPL_REPLY; } tor_assert(diff_from_nt); @@ -1497,11 +1509,11 @@ consensus_diff_worker_threadfn(void *state_, void *work_) // XXXX inputs again, even though we already have that. Maybe it's time // XXXX to change the API here? consensus_diff = consensus_diff_generate(diff_from_nt, - strlen(diff_from_nt), + diff_from_nt_len, diff_to_nt, - strlen(diff_to_nt)); - tor_free(diff_from_nt); - tor_free(diff_to_nt); + diff_to_nt_len); + tor_free(owned1); + tor_free(owned2); } if (!consensus_diff) { /* Couldn't generate consensus; we'll leave the reply blank. */ diff --git a/src/feature/dircache/consdiffmgr.h b/src/feature/dircache/consdiffmgr.h index 66c3d6500..d6f273cc4 100644 --- a/src/feature/dircache/consdiffmgr.h +++ b/src/feature/dircache/consdiffmgr.h @@ -68,8 +68,9 @@ STATIC consensus_cache_entry_t *cdm_cache_lookup_consensus( STATIC int cdm_entry_get_sha3_value(uint8_t *digest_out, consensus_cache_entry_t *ent, const char *label); -STATIC int uncompress_or_copy(char **out, size_t *outlen, - consensus_cache_entry_t *ent); +STATIC int uncompress_or_set_ptr(const char **out, size_t *outlen, + char **owned_out, + consensus_cache_entry_t *ent); #endif /* defined(CONSDIFFMGR_PRIVATE) */
#endif /* !defined(TOR_CONSDIFFMGR_H) */ diff --git a/src/test/fuzz/fuzz_diff.c b/src/test/fuzz/fuzz_diff.c index 8966856be..64aecc8a6 100644 --- a/src/test/fuzz/fuzz_diff.c +++ b/src/test/fuzz/fuzz_diff.c @@ -48,18 +48,27 @@ fuzz_main(const uint8_t *stdin_buf, size_t data_size) size_t c2_len = data_size - c1_len - SEPLEN; const char *c2 = (const char *)separator + SEPLEN;
+ const char *cp = memchr(c1, 0, c1_len); + if (cp) + c1_len = cp - c1; + + cp = memchr(c2, 0, c2_len); + if (cp) + c2_len = cp - c2; + char *c3 = consensus_diff_generate(c1, c1_len, c2, c2_len);
if (c3) { char *c4 = consensus_diff_apply(c1, c1_len, c3, strlen(c3)); tor_assert(c4); - if (strcmp(c2, c4)) { - printf("%s\n", escaped(c1)); - printf("%s\n", escaped(c2)); + int equal = (c2_len == strlen(c4)) && fast_memeq(c2, c4, c2_len); + if (! equal) { + //printf("%s\n", escaped(c1)); + //printf("%s\n", escaped(c2)); printf("%s\n", escaped(c3)); printf("%s\n", escaped(c4)); } - tor_assert(! strcmp(c2, c4)); + tor_assert(equal); tor_free(c3); tor_free(c4); } diff --git a/src/test/test_consdiffmgr.c b/src/test/test_consdiffmgr.c index dc4fea7f6..4b49fdb6a 100644 --- a/src/test/test_consdiffmgr.c +++ b/src/test/test_consdiffmgr.c @@ -191,14 +191,15 @@ lookup_apply_and_verify_diff(consensus_flavor_t flav,
consensus_cache_entry_incref(ent); size_t size; - char *diff_string = NULL; - int r = uncompress_or_copy(&diff_string, &size, ent); + const char *diff_string = NULL; + char *diff_owned = NULL; + int r = uncompress_or_set_ptr(&diff_string, &size, &diff_owned, ent); consensus_cache_entry_decref(ent); if (diff_string == NULL || r < 0) return -1;
- char *applied = consensus_diff_apply_(str1, diff_string); - tor_free(diff_string); + char *applied = consensus_diff_apply(str1, strlen(str1), diff_string, size); + tor_free(diff_owned); if (applied == NULL) return -1;
@@ -298,7 +299,8 @@ test_consdiffmgr_add(void *arg) (void) arg; time_t now = approx_time();
- char *body = NULL; + const char *body = NULL; + char *body_owned = NULL;
consensus_cache_entry_t *ent = NULL; networkstatus_t *ns_tmp = fake_ns_new(FLAV_NS, now); @@ -340,7 +342,7 @@ test_consdiffmgr_add(void *arg) tt_assert(ent); consensus_cache_entry_incref(ent); size_t s; - r = uncompress_or_copy(&body, &s, ent); + r = uncompress_or_set_ptr(&body, &s, &body_owned, ent); tt_int_op(r, OP_EQ, 0); tt_int_op(s, OP_EQ, 4); tt_mem_op(body, OP_EQ, "quux", 4); @@ -353,7 +355,7 @@ test_consdiffmgr_add(void *arg) networkstatus_vote_free(ns_tmp); teardown_capture_of_logs(); consensus_cache_entry_decref(ent); - tor_free(body); + tor_free(body_owned); }
static void