Stop memcpy'ing uncompressed consensuses when making diffs

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
+ * 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
+ * set <b>outlen</b> to hold their size, and set *<b>owned_out</b> to a pointer
- * <b>out</b> and set <b>outlen</b> to its length.  Return 0 on success,
+ * that the caller will need to free.  If not, just set *<b>out</b> and
- * -1 on failure.
+ * <b>outlen</b> to its extent in memory.  Return 0 on success, -1 on failure.
- *
+ **/
- * In all cases, the output is nul-terminated. */
 STATIC int
-uncompress_or_copy(char **out, size_t *outlen,
+uncompress_or_set_ptr(const char **out, size_t *outlen,
-                   consensus_cache_entry_t *ent)
+                      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,
+    if (uncompress_or_set_ptr(&diff_from_nt, &diff_from_nt_len, &owned1,
-                           job->diff_from) < 0) {
+                              job->diff_from) < 0) {
       return WQ_RPL_REPLY;
     }
-    if (uncompress_or_copy(&diff_to_nt, &diff_to_nt_len,
+    if (uncompress_or_set_ptr(&diff_to_nt, &diff_to_nt_len, &owned2,
-                           job->diff_to) < 0) {
+                              job->diff_to) < 0) {
-      tor_free(diff_from_nt);
+      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));
+                                             diff_to_nt_len);
-    tor_free(diff_from_nt);
+    tor_free(owned1);
-    tor_free(diff_to_nt);
+    tor_free(owned2);
   }
   if (!consensus_diff) {
     /* Couldn't generate consensus; we'll leave the reply blank. */

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,
+STATIC int uncompress_or_set_ptr(const char **out, size_t *outlen,
-                              consensus_cache_entry_t *ent);
+                                 char **owned_out,
+                                 consensus_cache_entry_t *ent);
 #endif /* defined(CONSDIFFMGR_PRIVATE) */
 
 #endif /* !defined(TOR_CONSDIFFMGR_H) */

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)) {
+    int equal = (c2_len == strlen(c4)) && fast_memeq(c2, c4, c2_len);
-      printf("%s\n", escaped(c1));
+    if (! equal) {
-      printf("%s\n", escaped(c2));
+      //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);
   }

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;
+  const char *diff_string = NULL;
-  int r = uncompress_or_copy(&diff_string, &size, ent);
+  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);
+  char *applied = consensus_diff_apply(str1, strlen(str1), diff_string, size);
-  tor_free(diff_string);
+  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