1 files changed, 76 insertions, 31 deletions

diff --git a/src/feature/dircache/consdiffmgr.c b/src/feature/dircache/consdiffmgr.c
index 025361fa60..21f536432c 100644
--- a/src/feature/dircache/consdiffmgr.c
+++ b/src/feature/dircache/consdiffmgr.c
@@ -1,8 +1,8 @@
-/* Copyright (c) 2017-2019, The Tor Project, Inc. */
+/* Copyright (c) 2017-2020, The Tor Project, Inc. */
 /* See LICENSE for licensing information */
 
 /**
- * \file consdiffmsr.c
+ * \file consdiffmgr.c
  *
  * \brief consensus diff manager functions
  *
@@ -177,6 +177,16 @@ typedef struct cdm_diff_t {
 /** Hashtable mapping flavor and source consensus digest to status. */
 static HT_HEAD(cdm_diff_ht, cdm_diff_t) cdm_diff_ht = HT_INITIALIZER();
 
+#ifdef _WIN32
+   // XXX(ahf): For tor#24857, a contributor suggested that on Windows, the CPU
+   // begins to spike at 100% once the number of files handled by the consensus
+   // diff manager becomes larger than 64. To see if the issue goes away, we
+   // hardcode this value to 64 now while we investigate a better solution.
+#  define CACHE_MAX_NUM 64
+#else
+#  define CACHE_MAX_NUM 128
+#endif
+
 /**
  * Configuration for this module
  */
@@ -184,11 +194,12 @@ static consdiff_cfg_t consdiff_cfg = {
   // XXXX I'd like to make this number bigger, but it interferes with the
   // XXXX seccomp2 syscall filter, which tops out at BPF_MAXINS (4096)
   // XXXX rules.
-  /* .cache_max_num = */ 128
+  /* .cache_max_num = */ CACHE_MAX_NUM
 };
 
 static int consdiffmgr_ensure_space_for_files(int n);
 static int consensus_queue_compression_work(const char *consensus,
+                                            size_t consensus_len,
                                             const networkstatus_t *as_parsed);
 static int consensus_diff_queue_diff_work(consensus_cache_entry_t *diff_from,
                                           consensus_cache_entry_t *diff_to);
@@ -217,9 +228,9 @@ cdm_diff_eq(const cdm_diff_t *diff1, const cdm_diff_t *diff2)
     diff1->compress_method == diff2->compress_method;
 }
 
-HT_PROTOTYPE(cdm_diff_ht, cdm_diff_t, node, cdm_diff_hash, cdm_diff_eq)
+HT_PROTOTYPE(cdm_diff_ht, cdm_diff_t, node, cdm_diff_hash, cdm_diff_eq);
 HT_GENERATE2(cdm_diff_ht, cdm_diff_t, node, cdm_diff_hash, cdm_diff_eq,
-             0.6, tor_reallocarray, tor_free_)
+             0.6, tor_reallocarray, tor_free_);
 
 #define cdm_diff_free(diff) \
   FREE_AND_NULL(cdm_diff_t, cdm_diff_free_, (diff))
@@ -509,8 +520,25 @@ get_max_age_to_cache(void)
                                         MAX_MAX_AGE_TO_CACHE);
 }
 
+#ifdef TOR_UNIT_TESTS
+/** As consdiffmgr_add_consensus, but requires a nul-terminated input. For
+ * testing. */
+int
+consdiffmgr_add_consensus_nulterm(const char *consensus,
+                                  const networkstatus_t *as_parsed)
+{
+  size_t len = strlen(consensus);
+  /* make a non-nul-terminated copy so that we can have a better chance
+   * of catching errors. */
+  char *ctmp = tor_memdup(consensus, len);
+  int r = consdiffmgr_add_consensus(ctmp, len, as_parsed);
+  tor_free(ctmp);
+  return r;
+}
+#endif /* defined(TOR_UNIT_TESTS) */
+
 /**
- * Given a string containing a networkstatus consensus, and the results of
+ * Given a buffer containing a networkstatus consensus, and the results of
  * having parsed that consensus, add that consensus to the cache if it is not
  * already present and not too old.  Create new consensus diffs from or to
  * that consensus as appropriate.
@@ -519,6 +547,7 @@ get_max_age_to_cache(void)
  */
 int
 consdiffmgr_add_consensus(const char *consensus,
+                          size_t consensus_len,
                           const networkstatus_t *as_parsed)
 {
   if (BUG(consensus == NULL) || BUG(as_parsed == NULL))
@@ -544,7 +573,7 @@ consdiffmgr_add_consensus(const char *consensus,
   }
 
   /* We don't have it. Add it to the cache. */
-  return consensus_queue_compression_work(consensus, as_parsed);
+  return consensus_queue_compression_work(consensus, consensus_len, as_parsed);
 }
 
 /**
@@ -825,7 +854,7 @@ consdiffmgr_configure(const consdiff_cfg_t *cfg)
  * operations that the consensus diff manager will need.
  */
 int
-consdiffmgr_register_with_sandbox(struct sandbox_cfg_elem **cfg)
+consdiffmgr_register_with_sandbox(struct sandbox_cfg_elem_t **cfg)
 {
   return consensus_cache_register_with_sandbox(cdm_cache_get(), cfg);
 }
@@ -1274,7 +1303,7 @@ typedef struct compressed_result_t {
 
 /**
  * Compress the bytestring <b>input</b> of length <b>len</b> using the
- * <n>n_methods</b> compression methods listed in the array <b>methods</b>.
+ * <b>n_methods</b> compression methods listed in the array <b>methods</b>.
  *
  * For each successful compression, set the fields in the <b>results_out</b>
  * array in the position corresponding to the compression method. Use
@@ -1387,19 +1416,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 +1441,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 +1518,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);
@@ -1496,9 +1537,12 @@ consensus_diff_worker_threadfn(void *state_, void *work_)
     // XXXX ugh; this is going to calculate the SHA3 of both its
     // 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, diff_to_nt);
-    tor_free(diff_from_nt);
-    tor_free(diff_to_nt);
+    consensus_diff = consensus_diff_generate(diff_from_nt,
+                                             diff_from_nt_len,
+                                             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. */
@@ -1746,8 +1790,8 @@ consensus_compress_worker_threadfn(void *state_, void *work_)
                           (const uint8_t *)consensus, bodylen);
   {
     const char *start, *end;
-    if (router_get_networkstatus_v3_signed_boundaries(consensus,
-                                                        &start, &end) < 0) {
+    if (router_get_networkstatus_v3_signed_boundaries(consensus, bodylen,
+                                                      &start, &end) < 0) {
       start = consensus;
       end = consensus+bodylen;
     }
@@ -1811,14 +1855,15 @@ static int background_compression = 0;
  */
 static int
 consensus_queue_compression_work(const char *consensus,
+                                 size_t consensus_len,
                                  const networkstatus_t *as_parsed)
 {
   tor_assert(consensus);
   tor_assert(as_parsed);
 
   consensus_compress_worker_job_t *job = tor_malloc_zero(sizeof(*job));
-  job->consensus = tor_strdup(consensus);
-  job->consensus_len = strlen(consensus);
+  job->consensus = tor_memdup_nulterm(consensus, consensus_len);
+  job->consensus_len = strlen(job->consensus);
   job->flavor = as_parsed->flavor;
 
   char va_str[ISO_TIME_LEN+1];