Merge remote-tracking branch 'andrea/bug18322_v3_squashed'

12 files changed, 1652 insertions, 38 deletions

diff --git a/changes/bug18322 b/changes/bug18322
new file mode 100644
index 0000000000..d85b9bf47f
--- /dev/null
+++ b/changes/bug18322
@@ -0,0 +1,4 @@
+  o Bugfixes:
+    - When dumping unparseable router descriptors, optionally store them in
+      separate filenames by hash, up to a configurable limit.
+      Fixes bug 18322; bugfix on ???.
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index b49ebfd121..64f0da0eeb 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -602,6 +602,13 @@ GENERAL OPTIONS
     message currently has at least one domain; most currently have exactly
     one.  This doesn't affect controller log messages. (Default: 0)
 
+[[MaxUnparseableDescSizeToLog]] **MaxUnparseableDescSizeToLog** __N__ **bytes**|**KBytes**|**MBytes**|**GBytes**::
+    Unparseable descriptors (e.g. for votes, consensuses, routers) are logged
+    in separate files by hash, up to the specified size in total.  Note that
+    only files logged during the lifetime of this Tor process count toward the
+    total; this is intended to be used to debug problems without opening live
+    servers to resource exhaustion attacks. (Default: 10 MB)
+
 [[OutboundBindAddress]] **OutboundBindAddress** __IP__::
     Make all outbound connections originate from the IP address specified. This
     is only useful when you have multiple network interfaces, and you want all
diff --git a/src/common/testsupport.h b/src/common/testsupport.h
index 3bb11a7e41..9ad2ba77e0 100644
--- a/src/common/testsupport.h
+++ b/src/common/testsupport.h
@@ -6,8 +6,10 @@
 
 #ifdef TOR_UNIT_TESTS
 #define STATIC
+#define EXTERN(type, name) extern type name;
 #else
 #define STATIC static
+#define EXTERN(type, name)
 #endif
 
 /** Quick and dirty macros to implement test mocking.
@@ -60,6 +62,12 @@
 #define MOCK_IMPL(rv, funcname, arglist)     \
   rv(*funcname) arglist = funcname ##__real; \
   rv funcname ##__real arglist
+#define MOCK_DECL_ATTR(rv, funcname, arglist, attr) \
+  rv funcname ##__real arglist attr;                \
+  extern rv(*funcname) arglist
+#define MOCK_IMPL(rv, funcname, arglist)     \
+  rv(*funcname) arglist = funcname ##__real; \
+  rv funcname ##__real arglist
 #define MOCK(func, replacement)                 \
   do {                                          \
     (func) = (replacement);                     \
@@ -71,6 +79,8 @@
 #else
 #define MOCK_DECL(rv, funcname, arglist) \
   rv funcname arglist
+#define MOCK_DECL_ATTR(rv, funcname, arglist, attr) \
+  rv funcname arglist attr
 #define MOCK_IMPL(rv, funcname, arglist) \
   rv funcname arglist
 #endif
diff --git a/src/common/util.c b/src/common/util.c
index fce99ee3f4..8a4fa1d710 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -2103,6 +2103,16 @@ clean_name_for_stat(char *name)
 #endif
 }
 
+/** Wrapper for unlink() to make it mockable for the test suite; returns 0
+ * if unlinking the file succeeded, -1 and sets errno if unlinking fails.
+ */
+
+MOCK_IMPL(int,
+tor_unlink,(const char *pathname))
+{
+  return unlink(pathname);
+}
+
 /** Return:
  * FN_ERROR if filename can't be read, is NULL, or is zero-length,
  * FN_NOENT if it doesn't exist,
@@ -2166,9 +2176,9 @@ file_status(const char *fname)
  * When effective_user is not NULL, check permissions against the given user
  * and its primary group.
  */
-int
-check_private_dir(const char *dirname, cpd_check_t check,
-                  const char *effective_user)
+MOCK_IMPL(int,
+check_private_dir,(const char *dirname, cpd_check_t check,
+                   const char *effective_user))
 {
   int r;
   struct stat st;
@@ -2396,8 +2406,8 @@ check_private_dir(const char *dirname, cpd_check_t check,
  * function, and all other functions in util.c that create files, create them
  * with mode 0600.
  */
-int
-write_str_to_file(const char *fname, const char *str, int bin)
+MOCK_IMPL(int,
+write_str_to_file,(const char *fname, const char *str, int bin))
 {
 #ifdef _WIN32
   if (!bin && strchr(str, '\r')) {
@@ -2756,8 +2766,8 @@ read_file_to_str_until_eof(int fd, size_t max_bytes_to_read, size_t *sz_out)
  * the call to stat and the call to read_all: the resulting string will
  * be truncated.
  */
-char *
-read_file_to_str(const char *filename, int flags, struct stat *stat_out)
+MOCK_IMPL(char *,
+read_file_to_str, (const char *filename, int flags, struct stat *stat_out))
 {
   int fd; /* router file */
   struct stat statbuf;
@@ -3492,8 +3502,8 @@ smartlist_add_vasprintf(struct smartlist_t *sl, const char *pattern,
 /** Return a new list containing the filenames in the directory <b>dirname</b>.
  * Return NULL on error or if <b>dirname</b> is not a directory.
  */
-smartlist_t *
-tor_listdir(const char *dirname)
+MOCK_IMPL(smartlist_t *,
+tor_listdir, (const char *dirname))
 {
   smartlist_t *result;
 #ifdef _WIN32
diff --git a/src/common/util.h b/src/common/util.h
index 7cb33dc680..44f510cef7 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -309,6 +309,8 @@ const char *stream_status_to_string(enum stream_status stream_status);
 
 enum stream_status get_string_from_pipe(FILE *stream, char *buf, size_t count);
 
+MOCK_DECL(int,tor_unlink,(const char *pathname));
+
 /** Return values from file_status(); see that function's documentation
  * for details. */
 typedef enum { FN_ERROR, FN_NOENT, FN_FILE, FN_DIR, FN_EMPTY } file_status_t;
@@ -324,8 +326,9 @@ typedef unsigned int cpd_check_t;
 #define CPD_GROUP_READ           (1u << 3)
 #define CPD_CHECK_MODE_ONLY      (1u << 4)
 #define CPD_RELAX_DIRMODE_CHECK  (1u << 5)
-int check_private_dir(const char *dirname, cpd_check_t check,
-                      const char *effective_user);
+MOCK_DECL(int, check_private_dir,
+    (const char *dirname, cpd_check_t check,
+     const char *effective_user));
 
 #define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
 #define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
@@ -338,7 +341,8 @@ FILE *start_writing_to_stdio_file(const char *fname, int open_flags, int mode,
 FILE *fdopen_file(open_file_t *file_data);
 int finish_writing_to_file(open_file_t *file_data);
 int abort_writing_to_file(open_file_t *file_data);
-int write_str_to_file(const char *fname, const char *str, int bin);
+MOCK_DECL(int,
+write_str_to_file,(const char *fname, const char *str, int bin));
 MOCK_DECL(int,
 write_bytes_to_file,(const char *fname, const char *str, size_t len,
                      int bin));
@@ -363,8 +367,9 @@ int write_bytes_to_new_file(const char *fname, const char *str, size_t len,
 #ifndef _WIN32
 struct stat;
 #endif
-char *read_file_to_str(const char *filename, int flags, struct stat *stat_out)
-  ATTR_MALLOC;
+MOCK_DECL_ATTR(char *, read_file_to_str,
+               (const char *filename, int flags, struct stat *stat_out),
+               ATTR_MALLOC);
 char *read_file_to_str_until_eof(int fd, size_t max_bytes_to_read,
                                  size_t *sz_out)
   ATTR_MALLOC;
@@ -372,7 +377,7 @@ const char *parse_config_line_from_str_verbose(const char *line,
                                        char **key_out, char **value_out,
                                        const char **err_out);
 char *expand_filename(const char *filename);
-struct smartlist_t *tor_listdir(const char *dirname);
+MOCK_DECL(struct smartlist_t *, tor_listdir, (const char *dirname));
 int path_is_relative(const char *filename);
 
 /* Process helpers */
diff --git a/src/or/config.c b/src/or/config.c
index cdd4f10e92..18ee9790f6 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -325,6 +325,7 @@ static config_var_t option_vars_[] = {
   VAR("MaxMemInQueues",          MEMUNIT,   MaxMemInQueues_raw, "0"),
   OBSOLETE("MaxOnionsPending"),
   V(MaxOnionQueueDelay,          MSEC_INTERVAL, "1750 msec"),
+  V(MaxUnparseableDescSizeToLog, MEMUNIT, "10 MB"),
   V(MinMeasuredBWsForAuthToIgnoreAdvertised, INT, "500"),
   V(MyFamily,                    STRING,   NULL),
   V(NewCircuitPeriod,            INTERVAL, "30 seconds"),
@@ -7228,10 +7229,10 @@ init_libevent(const or_options_t *options)
  *
  * Note: Consider using the get_datadir_fname* macros in or.h.
  */
-char *
-options_get_datadir_fname2_suffix(const or_options_t *options,
-                                  const char *sub1, const char *sub2,
-                                  const char *suffix)
+MOCK_IMPL(char *,
+options_get_datadir_fname2_suffix,(const or_options_t *options,
+                                   const char *sub1, const char *sub2,
+                                   const char *suffix))
 {
   char *fname = NULL;
   size_t len;
diff --git a/src/or/config.h b/src/or/config.h
index e08ad81304..a0fe6e4805 100644
--- a/src/or/config.h
+++ b/src/or/config.h
@@ -53,9 +53,11 @@ config_line_t *option_get_assignment(const or_options_t *options,
                                      const char *key);
 int options_save_current(void);
 const char *get_torrc_fname(int defaults_fname);
-char *options_get_datadir_fname2_suffix(const or_options_t *options,
-                                        const char *sub1, const char *sub2,
-                                        const char *suffix);
+MOCK_DECL(char *,
+          options_get_datadir_fname2_suffix,
+          (const or_options_t *options,
+           const char *sub1, const char *sub2,
+           const char *suffix));
 #define get_datadir_fname2_suffix(sub1, sub2, suffix) \
   options_get_datadir_fname2_suffix(get_options(), (sub1), (sub2), (suffix))
 /** Return a newly allocated string containing datadir/sub1.  See
diff --git a/src/or/main.c b/src/or/main.c
index 4de2e70a1d..65a67a9923 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -3045,6 +3045,9 @@ tor_init(int argc, char *argv[])
     log_warn(LD_NET, "Problem initializing libevent RNG.");
   }
 
+  /* Scan/clean unparseable descroptors; after reading config */
+  routerparse_init();
+
   return 0;
 }
 
@@ -3146,6 +3149,7 @@ tor_free_all(int postfork)
   scheduler_free_all();
   nodelist_free_all();
   microdesc_free_all();
+  routerparse_free_all();
   ext_orport_free_all();
   control_free_all();
   sandbox_free_getaddrinfo_cache();
diff --git a/src/or/or.h b/src/or/or.h
index ea38022f43..a1a0810e4b 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -4498,6 +4498,11 @@ typedef struct {
 
   /** Autobool: Do we try to retain capabilities if we can? */
   int KeepBindCapabilities;
+
+  /** Maximum total size of unparseable descriptors to log during the
+   * lifetime of this Tor process.
+   */
+  uint64_t MaxUnparseableDescSizeToLog;
 } or_options_t;
 
 /** Persistent state for an onion router, as saved to disk. */
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index 6550fa6715..875bb605b0 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -28,6 +28,7 @@
 #include "routerparse.h"
 #include "entrynodes.h"
 #include "torcert.h"
+#include "sandbox.h"
 
 #undef log
 #include <math.h>
@@ -585,32 +586,561 @@ static int check_signature_token(const char *digest,
 #define DUMP_AREA(a,name) STMT_NIL
 #endif
 
-/** Last time we dumped a descriptor to disk. */
-static time_t last_desc_dumped = 0;
+/* Dump mechanism for unparseable descriptors */
+
+/** List of dumped descriptors for FIFO cleanup purposes */
+STATIC smartlist_t *descs_dumped = NULL;
+/** Total size of dumped descriptors for FIFO cleanup */
+STATIC uint64_t len_descs_dumped = 0;
+/** Directory to stash dumps in */
+static int have_dump_desc_dir = 0;
+static int problem_with_dump_desc_dir = 0;
+
+#define DESC_DUMP_DATADIR_SUBDIR "unparseable-descs"
+#define DESC_DUMP_BASE_FILENAME "unparseable-desc"
+
+/** Find the dump directory and check if we'll be able to create it */
+static void
+dump_desc_init(void)
+{
+  char *dump_desc_dir;
+
+  dump_desc_dir = get_datadir_fname(DESC_DUMP_DATADIR_SUBDIR);
+
+  /*
+   * We just check for it, don't create it at this point; we'll
+   * create it when we need it if it isn't already there.
+   */
+  if (check_private_dir(dump_desc_dir, CPD_CHECK, get_options()->User) < 0) {
+    /* Error, log and flag it as having a problem */
+    log_notice(LD_DIR,
+               "Doesn't look like we'll be able to create descriptor dump "
+               "directory %s; dumps will be disabled.",
+               dump_desc_dir);
+    problem_with_dump_desc_dir = 1;
+    tor_free(dump_desc_dir);
+    return;
+  }
+
+  /* Check if it exists */
+  switch (file_status(dump_desc_dir)) {
+    case FN_DIR:
+      /* We already have a directory */
+      have_dump_desc_dir = 1;
+      break;
+    case FN_NOENT:
+      /* Nothing, we'll need to create it later */
+      have_dump_desc_dir = 0;
+      break;
+    case FN_ERROR:
+      /* Log and flag having a problem */
+      log_notice(LD_DIR,
+                 "Couldn't check whether descriptor dump directory %s already"
+                 " exists: %s",
+                 dump_desc_dir, strerror(errno));
+      problem_with_dump_desc_dir = 1;
+    case FN_FILE:
+    case FN_EMPTY:
+    default:
+      /* Something else was here! */
+      log_notice(LD_DIR,
+                 "Descriptor dump directory %s already exists and isn't a "
+                 "directory",
+                 dump_desc_dir);
+      problem_with_dump_desc_dir = 1;
+  }
+
+  if (have_dump_desc_dir && !problem_with_dump_desc_dir) {
+    dump_desc_populate_fifo_from_directory(dump_desc_dir);
+  }
+
+  tor_free(dump_desc_dir);
+}
+
+/** Create the dump directory if needed and possible */
+static void
+dump_desc_create_dir(void)
+{
+  char *dump_desc_dir;
+
+  /* If the problem flag is set, skip it */
+  if (problem_with_dump_desc_dir) return;
+
+  /* Do we need it? */
+  if (!have_dump_desc_dir) {
+    dump_desc_dir = get_datadir_fname(DESC_DUMP_DATADIR_SUBDIR);
+
+    if (check_private_dir(dump_desc_dir, CPD_CREATE,
+                          get_options()->User) < 0) {
+      log_notice(LD_DIR,
+                 "Failed to create descriptor dump directory %s",
+                 dump_desc_dir);
+      problem_with_dump_desc_dir = 1;
+    }
+
+    /* Okay, we created it */
+    have_dump_desc_dir = 1;
+
+    tor_free(dump_desc_dir);
+  }
+}
+
+/** Dump desc FIFO/cleanup; take ownership of the given filename, add it to
+ * the FIFO, and clean up the oldest entries to the extent they exceed the
+ * configured cap.  If any old entries with a matching hash existed, they
+ * just got overwritten right before this was called and we should adjust
+ * the total size counter without deleting them.
+ */
+static void
+dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256,
+                             size_t len)
+{
+  dumped_desc_t *ent = NULL, *tmp;
+  uint64_t max_len;
+
+  tor_assert(filename != NULL);
+  tor_assert(digest_sha256 != NULL);
+
+  if (descs_dumped == NULL) {
+    /* We better have no length, then */
+    tor_assert(len_descs_dumped == 0);
+    /* Make a smartlist */
+    descs_dumped = smartlist_new();
+  }
+
+  /* Make a new entry to put this one in */
+  ent = tor_malloc_zero(sizeof(*ent));
+  ent->filename = filename;
+  ent->len = len;
+  ent->when = time(NULL);
+  memcpy(ent->digest_sha256, digest_sha256, DIGEST256_LEN);
+
+  /* Do we need to do some cleanup? */
+  max_len = get_options()->MaxUnparseableDescSizeToLog;
+  /* Iterate over the list until we've freed enough space */
+  while (len > max_len - len_descs_dumped &&
+         smartlist_len(descs_dumped) > 0) {
+    /* Get the oldest thing on the list */
+    tmp = (dumped_desc_t *)(smartlist_get(descs_dumped, 0));
+
+    /*
+     * Check if it matches the filename we just added, so we don't delete
+     * something we just emitted if we get repeated identical descriptors.
+     */
+    if (strcmp(tmp->filename, filename) != 0) {
+      /* Delete it and adjust the length counter */
+      tor_unlink(tmp->filename);
+      tor_assert(len_descs_dumped >= tmp->len);
+      len_descs_dumped -= tmp->len;
+      log_info(LD_DIR,
+               "Deleting old unparseable descriptor dump %s due to "
+               "space limits",
+               tmp->filename);
+    } else {
+      /*
+       * Don't delete, but do adjust the counter since we will bump it
+       * later
+       */
+      tor_assert(len_descs_dumped >= tmp->len);
+      len_descs_dumped -= tmp->len;
+      log_info(LD_DIR,
+               "Replacing old descriptor dump %s with new identical one",
+               tmp->filename);
+    }
+
+    /* Free it and remove it from the list */
+    smartlist_del_keeporder(descs_dumped, 0);
+    tor_free(tmp->filename);
+    tor_free(tmp);
+  }
+
+  /* Append our entry to the end of the list and bump the counter */
+  smartlist_add(descs_dumped, ent);
+  len_descs_dumped += len;
+}
+
+/** Check if we already have a descriptor for this hash and move it to the
+ * head of the queue if so.  Return 1 if one existed and 0 otherwise.
+ */
+static int
+dump_desc_fifo_bump_hash(const uint8_t *digest_sha256)
+{
+  dumped_desc_t *match = NULL;
+
+  tor_assert(digest_sha256);
+
+  if (descs_dumped) {
+    /* Find a match if one exists */
+    SMARTLIST_FOREACH_BEGIN(descs_dumped, dumped_desc_t *, ent) {
+      if (ent &&
+          memcmp(ent->digest_sha256, digest_sha256, DIGEST256_LEN) == 0) {
+        /*
+         * Save a pointer to the match and remove it from its current
+         * position.
+         */
+        match = ent;
+        SMARTLIST_DEL_CURRENT_KEEPORDER(descs_dumped, ent);
+        break;
+      }
+    } SMARTLIST_FOREACH_END(ent);
+
+    if (match) {
+      /* Update the timestamp */
+      match->when = time(NULL);
+      /* Add it back at the end of the list */
+      smartlist_add(descs_dumped, match);
+
+      /* Indicate we found one */
+      return 1;
+    }
+  }
+
+  return 0;
+}
+
+/** Clean up on exit; just memory, leave the dumps behind
+ */
+STATIC void
+dump_desc_fifo_cleanup(void)
+{
+  if (descs_dumped) {
+    /* Free each descriptor */
+    SMARTLIST_FOREACH_BEGIN(descs_dumped, dumped_desc_t *, ent) {
+      tor_assert(ent);
+      tor_free(ent->filename);
+      tor_free(ent);
+    } SMARTLIST_FOREACH_END(ent);
+    /* Free the list */
+    smartlist_free(descs_dumped);
+    descs_dumped = NULL;
+    len_descs_dumped = 0;
+  }
+}
+
+/** Handle one file for dump_desc_populate_fifo_from_directory(); make sure
+ * the filename is sensibly formed and matches the file content, and either
+ * return a dumped_desc_t for it or remove the file and return NULL.
+ */
+MOCK_IMPL(STATIC dumped_desc_t *,
+dump_desc_populate_one_file, (const char *dirname, const char *f))
+{
+  dumped_desc_t *ent = NULL;
+  char *path = NULL, *desc = NULL;
+  const char *digest_str;
+  char digest[DIGEST256_LEN], content_digest[DIGEST256_LEN];
+  /* Expected prefix before digest in filenames */
+  const char *f_pfx = DESC_DUMP_BASE_FILENAME ".";
+  /*
+   * Stat while reading; this is important in case the file
+   * contains a NUL character.
+   */
+  struct stat st;
+
+  /* Sanity-check args */
+  tor_assert(dirname != NULL);
+  tor_assert(f != NULL);
+
+  /* Form the full path */
+  tor_asprintf(&path, "%s" PATH_SEPARATOR "%s", dirname, f);
+
+  /* Check that f has the form DESC_DUMP_BASE_FILENAME.<digest256> */
+
+  if (!strcmpstart(f, f_pfx)) {
+    /* It matches the form, but is the digest parseable as such? */
+    digest_str = f + strlen(f_pfx);
+    if (base16_decode(digest, DIGEST256_LEN,
+                      digest_str, strlen(digest_str)) != DIGEST256_LEN) {
+      /* We failed to decode it */
+      digest_str = NULL;
+    }
+  } else {
+    /* No match */
+    digest_str = NULL;
+  }
+
+  if (!digest_str) {
+    /* We couldn't get a sensible digest */
+    log_notice(LD_DIR,
+               "Removing unrecognized filename %s from unparseable "
+               "descriptors directory", f);
+    tor_unlink(path);
+    /* We're done */
+    goto done;
+  }
+
+  /*
+   * The filename has the form DESC_DUMP_BASE_FILENAME "." <digest256> and
+   * we've decoded the digest.  Next, check that we can read it and the
+   * content matches this digest.  We are relying on the fact that if the
+   * file contains a '\0', read_file_to_str() will allocate space for and
+   * read the entire file and return the correct size in st.
+   */
+  desc = read_file_to_str(path, RFTS_IGNORE_MISSING, &st);
+  if (!desc) {
+    /* We couldn't read it */
+    log_notice(LD_DIR,
+               "Failed to read %s from unparseable descriptors directory; "
+               "attempting to remove it.", f);
+    tor_unlink(path);
+    /* We're done */
+    goto done;
+  }
+
+  /*
+   * We got one; now compute its digest and check that it matches the
+   * filename.
+   */
+  if (crypto_digest256((char *)content_digest, desc, st.st_size,
+                       DIGEST_SHA256) != 0) {
+    /* Weird, but okay */
+    log_info(LD_DIR,
+             "Unable to hash content of %s from unparseable descriptors "
+             "directory", f);
+    tor_unlink(path);
+    /* We're done */
+    goto done;
+  }
+
+  /* Compare the digests */
+  if (memcmp(digest, content_digest, DIGEST256_LEN) != 0) {
+    /* No match */
+    log_info(LD_DIR,
+             "Hash of %s from unparseable descriptors directory didn't "
+             "match its filename; removing it", f);
+    tor_unlink(path);
+    /* We're done */
+    goto done;
+  }
+
+  /* Okay, it's a match, we should prepare ent */
+  ent = tor_malloc_zero(sizeof(dumped_desc_t));
+  ent->filename = path;
+  memcpy(ent->digest_sha256, digest, DIGEST256_LEN);
+  ent->len = st.st_size;
+  ent->when = st.st_mtime;
+  /* Null out path so we don't free it out from under ent */
+  path = NULL;
+
+ done:
+  /* Free allocations if we had them */
+  tor_free(desc);
+  tor_free(path);
+
+  return ent;
+}
+
+/** Sort helper for dump_desc_populate_fifo_from_directory(); compares
+ * the when field of dumped_desc_ts in a smartlist to put the FIFO in
+ * the correct order after reconstructing it from the directory.
+ */
+static int
+dump_desc_compare_fifo_entries(const void **a_v, const void **b_v)
+{
+  const dumped_desc_t **a = (const dumped_desc_t **)a_v;
+  const dumped_desc_t **b = (const dumped_desc_t **)b_v;
+
+  if ((a != NULL) && (*a != NULL)) {
+    if ((b != NULL) && (*b != NULL)) {
+      /* We have sensible dumped_desc_ts to compare */
+      if ((*a)->when < (*b)->when) {
+        return -1;
+      } else if ((*a)->when == (*b)->when) {
+        return 0;
+      } else {
+        return 1;
+      }
+    } else {
+      /*
+       * We shouldn't see this, but what the hell, NULLs precede everythin
+       * else
+       */
+      return 1;
+    }
+  } else {
+    return -1;
+  }
+}
+
+/** Scan the contents of the directory, and update FIFO/counters; this will
+ * consistency-check descriptor dump filenames against hashes of descriptor
+ * dump file content, and remove any inconsistent/unreadable dumps, and then
+ * reconstruct the dump FIFO as closely as possible for the last time the
+ * tor process shut down.  If a previous dump was repeated more than once and
+ * moved ahead in the FIFO, the mtime will not have been updated and the
+ * reconstructed order will be wrong, but will always be a permutation of
+ * the original.
+ */
+STATIC void
+dump_desc_populate_fifo_from_directory(const char *dirname)
+{
+  smartlist_t *files = NULL;
+  dumped_desc_t *ent = NULL;
+
+  tor_assert(dirname != NULL);
+
+  /* Get a list of files */
+  files = tor_listdir(dirname);
+  if (!files) {
+    log_notice(LD_DIR,
+               "Unable to get contents of unparseable descriptor dump "
+               "directory %s",
+               dirname);
+    return;
+  }
+
+  /*
+   * Iterate through the list and decide which files should go in the
+   * FIFO and which should be purged.
+   */
+
+  SMARTLIST_FOREACH_BEGIN(files, char *, f) {
+    /* Try to get a FIFO entry */
+    ent = dump_desc_populate_one_file(dirname, f);
+    if (ent) {
+      /*
+       * We got one; add it to the FIFO.  No need for duplicate checking
+       * here since we just verified the name and digest match.
+       */
+
+      /* Make sure we have a list to add it to */
+      if (!descs_dumped) {
+        descs_dumped = smartlist_new();
+        len_descs_dumped = 0;
+      }
+
+      /* Add it and adjust the counter */
+      smartlist_add(descs_dumped, ent);
+      len_descs_dumped += ent->len;
+    }
+    /*
+     * If we didn't, we will have unlinked the file if necessary and
+     * possible, and emitted a log message about it, so just go on to
+     * the next.
+     */
+  } SMARTLIST_FOREACH_END(f);
+
+  /* Did we get anything? */
+  if (descs_dumped != NULL) {
+    /* Sort the FIFO in order of increasing timestamp */
+    smartlist_sort(descs_dumped, dump_desc_compare_fifo_entries);
+
+    /* Log some stats */
+    log_info(LD_DIR,
+             "Reloaded unparseable descriptor dump FIFO with %d dump(s) "
+             "totaling " U64_FORMAT " bytes",
+             smartlist_len(descs_dumped), U64_PRINTF_ARG(len_descs_dumped));
+  }
+
+  /* Free the original list */
+  SMARTLIST_FOREACH(files, char *, f, tor_free(f));
+  smartlist_free(files);
+}
 
 /** For debugging purposes, dump unparseable descriptor *<b>desc</b> of
  * type *<b>type</b> to file $DATADIR/unparseable-desc. Do not write more
  * than one descriptor to disk per minute. If there is already such a
  * file in the data directory, overwrite it. */
-static void
+STATIC void
 dump_desc(const char *desc, const char *type)
 {
-  time_t now = time(NULL);
   tor_assert(desc);
   tor_assert(type);
-  if (!last_desc_dumped || last_desc_dumped + 60 < now) {
-    char *debugfile = get_datadir_fname("unparseable-desc");
-    size_t filelen = 50 + strlen(type) + strlen(desc);
-    char *content = tor_malloc_zero(filelen);
-    tor_snprintf(content, filelen, "Unable to parse descriptor of type "
-                 "%s:\n%s", type, desc);
-    write_str_to_file(debugfile, content, 1);
-    log_info(LD_DIR, "Unable to parse descriptor of type %s. See file "
-             "unparseable-desc in data directory for details.", type);
-    tor_free(content);
-    tor_free(debugfile);
-    last_desc_dumped = now;
+  size_t len;
+  /* The SHA256 of the string */
+  uint8_t digest_sha256[DIGEST256_LEN];
+  char digest_sha256_hex[HEX_DIGEST256_LEN+1];
+  /* Filename to log it to */
+  char *debugfile, *debugfile_base;
+
+  /* Get the hash for logging purposes anyway */
+  len = strlen(desc);
+  if (crypto_digest256((char *)digest_sha256, desc, len,
+                       DIGEST_SHA256) != 0) {
+    log_info(LD_DIR,
+             "Unable to parse descriptor of type %s, and unable to even hash"
+             " it!", type);
+    goto err;
+  }
+
+  base16_encode(digest_sha256_hex, sizeof(digest_sha256_hex),
+                (const char *)digest_sha256, sizeof(digest_sha256));
+
+  /*
+   * We mention type and hash in the main log; don't clutter up the files
+   * with anything but the exact dump.
+   */
+  tor_asprintf(&debugfile_base,
+               DESC_DUMP_BASE_FILENAME ".%s", digest_sha256_hex);
+  debugfile = get_datadir_fname2(DESC_DUMP_DATADIR_SUBDIR, debugfile_base);
+
+  /*
+   * Check if the sandbox is active or will become active; see comment
+   * below at the log message for why.
+   */
+  if (!(sandbox_is_active() || get_options()->Sandbox)) {
+    if (len <= get_options()->MaxUnparseableDescSizeToLog) {
+      if (!dump_desc_fifo_bump_hash(digest_sha256)) {
+        /* Create the directory if needed */
+        dump_desc_create_dir();
+        /* Make sure we've got it */
+        if (have_dump_desc_dir && !problem_with_dump_desc_dir) {
+          /* Write it, and tell the main log about it */
+          write_str_to_file(debugfile, desc, 1);
+          log_info(LD_DIR,
+                   "Unable to parse descriptor of type %s with hash %s and "
+                   "length %lu. See file %s in data directory for details.",
+                   type, digest_sha256_hex, (unsigned long)len,
+                   debugfile_base);
+          dump_desc_fifo_add_and_clean(debugfile, digest_sha256, len);
+          /* Since we handed ownership over, don't free debugfile later */
+          debugfile = NULL;
+        } else {
+          /* Problem with the subdirectory */
+          log_info(LD_DIR,
+                   "Unable to parse descriptor of type %s with hash %s and "
+                   "length %lu. Descriptor not dumped because we had a "
+                   "problem creating the " DESC_DUMP_DATADIR_SUBDIR
+                   " subdirectory",
+                   type, digest_sha256_hex, (unsigned long)len);
+          /* We do have to free debugfile in this case */
+        }
+      } else {
+        /* We already had one with this hash dumped */
+        log_info(LD_DIR,
+                 "Unable to parse descriptor of type %s with hash %s and "
+                 "length %lu. Descriptor not dumped because one with that "
+                 "hash has already been dumped.",
+                 type, digest_sha256_hex, (unsigned long)len);
+        /* We do have to free debugfile in this case */
+      }
+    } else {
+      /* Just log that it happened without dumping */
+      log_info(LD_DIR,
+               "Unable to parse descriptor of type %s with hash %s and "
+               "length %lu. Descriptor not dumped because it exceeds maximum"
+               " log size all by itself.",
+               type, digest_sha256_hex, (unsigned long)len);
+      /* We do have to free debugfile in this case */
+    }
+  } else {
+    /*
+     * Not logging because the sandbox is active and seccomp2 apparently
+     * doesn't have a sensible way to allow filenames according to a pattern
+     * match.  (If we ever figure out how to say "allow writes to /regex/",
+     * remove this checK).
+     */
+    log_info(LD_DIR,
+             "Unable to parse descriptor of type %s with hash %s and "
+             "length %lu. Descriptor not dumped because the sandbox is "
+             "configured",
+             type, digest_sha256_hex, (unsigned long)len);
   }
+
+  tor_free(debugfile_base);
+  tor_free(debugfile);
+
+ err:
+  return;
 }
 
 /** Set <b>digest</b> to the SHA-1 digest of the hash of the directory in
@@ -5468,3 +5998,27 @@ rend_parse_client_keys(strmap_t *parsed_clients, const char *ckstr)
   return result;
 }
 
+/** Called on startup; right now we just handle scanning the unparseable
+ * descriptor dumps, but hang anything else we might need to do in the
+ * future here as well.
+ */
+void
+routerparse_init(void)
+{
+  /*
+   * Check both if the sandbox is active and whether it's configured; no
+   * point in loading all that if we won't be able to use it after the
+   * sandbox becomes active.
+   */
+  if (!(sandbox_is_active() || get_options()->Sandbox)) {
+    dump_desc_init();
+  }
+}
+
+/** Clean up all data structures used by routerparse.c at exit */
+void
+routerparse_free_all(void)
+{
+  dump_desc_fifo_cleanup();
+}
+
diff --git a/src/or/routerparse.h b/src/or/routerparse.h
index c46eb1c0ae..2eb9932410 100644
--- a/src/or/routerparse.h
+++ b/src/or/routerparse.h
@@ -85,11 +85,33 @@ int rend_parse_introduction_points(rend_service_descriptor_t *parsed,
                                    size_t intro_points_encoded_size);
 int rend_parse_client_keys(strmap_t *parsed_clients, const char *str);
 
+void routerparse_init(void);
+void routerparse_free_all(void);
+
 #ifdef ROUTERPARSE_PRIVATE
+/*
+ * One entry in the list of dumped descriptors; filename dumped to, length,
+ * SHA-256 and timestamp.
+ */
+
+typedef struct {
+  char *filename;
+  size_t len;
+  uint8_t digest_sha256[DIGEST256_LEN];
+  time_t when;
+} dumped_desc_t;
+
+EXTERN(size_t, len_descs_dumped);
+EXTERN(smartlist_t *, descs_dumped);
 STATIC int routerstatus_parse_guardfraction(const char *guardfraction_str,
                                             networkstatus_t *vote,
                                             vote_routerstatus_t *vote_rs,
                                             routerstatus_t *rs);
+MOCK_DECL(STATIC dumped_desc_t *, dump_desc_populate_one_file,
+    (const char *dirname, const char *f));
+STATIC void dump_desc_populate_fifo_from_directory(const char *dirname);
+STATIC void dump_desc(const char *desc, const char *type);
+STATIC void dump_desc_fifo_cleanup(void);
 #endif
 
 #define ED_DESC_SIGNATURE_PREFIX "Tor router descriptor signature v1"
diff --git a/src/test/test_dir.c b/src/test/test_dir.c
index b8dcab39d5..873426c32b 100644
--- a/src/test/test_dir.c
+++ b/src/test/test_dir.c
@@ -11,6 +11,7 @@
 #define DIRVOTE_PRIVATE
 #define ROUTER_PRIVATE
 #define ROUTERLIST_PRIVATE
+#define ROUTERPARSE_PRIVATE
 #define HIBERNATE_PRIVATE
 #define NETWORKSTATUS_PRIVATE
 #define RELAY_PRIVATE
@@ -4101,6 +4102,992 @@ test_dir_choose_compression_level(void* data)
   done: ;
 }
 
+/*
+ * Mock check_private_dir(), and always succeed - no need to actually
+ * look at or create anything on the filesystem.
+ */
+
+static int
+mock_check_private_dir(const char *dirname, cpd_check_t check,
+                       const char *effective_user)
+{
+  (void)dirname;
+  (void)check;
+  (void)effective_user;
+
+  return 0;
+}
+
+/*
+ * This really mocks options_get_datadir_fname2_suffix(), but for testing
+ * dump_desc(), we only care about get_datadir_fname(sub1), which is defined
+ * in config.h as:
+ *
+ * options_get_datadir_fname2_suffix(get_options(), sub1, NULL, NULL)
+ */
+
+static char *
+mock_get_datadir_fname(const or_options_t *options,
+                       const char *sub1, const char *sub2,
+                       const char *suffix)
+{
+  char *rv = NULL;
+
+  /*
+   * Assert we were called like get_datadir_fname2() or get_datadir_fname(),
+   * since that's all we implement here.
+   */
+  tt_assert(options != NULL);
+  tt_assert(sub1 != NULL);
+  /*
+   * No particular assertions about sub2, since we could be in the
+   * get_datadir_fname() or get_datadir_fname2() case.
+   */
+  tt_assert(suffix == NULL);
+
+  /* Just duplicate the basename and return it for this mock */
+  if (sub2) {
+    /* If we have sub2, it's the basename, otherwise sub1 */
+    rv = strdup(sub2);
+  } else {
+    rv = strdup(sub1);
+  }
+
+ done:
+  return rv;
+}
+
+static char *last_unlinked_path = NULL;
+static int unlinked_count = 0;
+
+static void
+mock_unlink_reset(void)
+{
+  tor_free(last_unlinked_path);
+  unlinked_count = 0;
+}
+
+static int
+mock_unlink(const char *path)
+{
+  tt_assert(path != NULL);
+
+  tor_free(last_unlinked_path);
+  last_unlinked_path = tor_strdup(path);
+  ++unlinked_count;
+
+ done:
+  return 0;
+}
+
+static char *last_write_str_path = NULL;
+static uint8_t last_write_str_hash[DIGEST256_LEN];
+static int write_str_count = 0;
+
+static void
+mock_write_str_to_file_reset(void)
+{
+  tor_free(last_write_str_path);
+  write_str_count = 0;
+}
+
+static int
+mock_write_str_to_file(const char *path, const char *str, int bin)
+{
+  size_t len;
+  uint8_t hash[DIGEST256_LEN];
+
+  (void)bin;
+
+  tt_assert(path != NULL);
+  tt_assert(str != NULL);
+
+  len = strlen(str);
+  crypto_digest256((char *)hash, str, len, DIGEST_SHA256);
+
+  tor_free(last_write_str_path);
+  last_write_str_path = tor_strdup(path);
+  memcpy(last_write_str_hash, hash, sizeof(last_write_str_hash));
+  ++write_str_count;
+
+ done:
+  return 0;
+}
+
+static void
+test_dir_dump_unparseable_descriptors(void *data)
+{
+  /*
+   * These bogus descriptors look nothing at all like real bogus descriptors
+   * we might see, but we're only testing dump_desc() here, not the parser.
+   */
+  const char *test_desc_type = "squamous";
+  /* strlen(test_desc_1) = 583 bytes */
+  const char *test_desc_1 =
+    "The most merciful thing in the world, I think, is the inability of the "
+    "human mind to correlate all its contents. We live on a placid island of"
+    " ignorance in the midst of black seas of infinity, and it was not meant"
+    " that we should voyage far. The sciences, each straining in its own dir"
+    "ection, have hitherto harmed us little; but some day the piecing togeth"
+    "er of dissociated knowledge will open up such terrifying vistas of real"
+    "ity, and of our frightful position therein, that we shall either go mad"
+    "from the revelation or flee from the light into the peace and safety of"
+    "a new dark age.";
+  uint8_t test_desc_1_hash[DIGEST256_LEN];
+  char test_desc_1_hash_str[HEX_DIGEST256_LEN+1];
+  /* strlen(test_desc_2) = 650 bytes */
+  const char *test_desc_2 =
+    "I think their predominant colour was a greyish-green, though they had w"
+    "hite bellies. They were mostly shiny and slippery, but the ridges of th"
+    "eir backs were scaly. Their forms vaguely suggested the anthropoid, whi"
+    "le their heads were the heads of fish, with prodigious bulging eyes tha"
+    "t never closed. At the sides of their necks were palpitating gills, and"
+    "their long paws were webbed. They hopped irregularly, sometimes on two "
+    "legs and sometimes on four. I was somehow glad that they had no more th"
+    "an four limbs. Their croaking, baying voices, clearly wed tar articulat"
+    "e speech, held all the dark shades of expression which their staring fa"
+    "ces lacked.";
+  uint8_t test_desc_2_hash[DIGEST256_LEN];
+  char test_desc_2_hash_str[HEX_DIGEST256_LEN+1];
+  /* strlen(test_desc_3) = 700 bytes */
+  const char *test_desc_3 =
+    "Without knowing what futurism is like, Johansen achieved something very"
+    "close to it when he spoke of the city; for instead of describing any de"
+    "finite structure or building, he dwells only on broad impressions of va"
+    "st angles and stone surfaces - surfaces too great to belong to anything"
+    "right or proper for this earth, and impious with horrible images and hi"
+    "eroglyphs. I mention his talk about angles because it suggests somethin"
+    "g Wilcox had told me of his awful dreams. He said that the geometry of "
+    "the dream-place he saw was abnormal, non-Euclidean, and loathsomely red"
+    "olent of spheres and dimensions apart from ours. Now an unlettered seam"
+    "an felt the same thing whilst gazing at the terrible reality.";
+  uint8_t test_desc_3_hash[DIGEST256_LEN];
+  char test_desc_3_hash_str[HEX_DIGEST256_LEN+1];
+  /* strlen(test_desc_3) = 604 bytes */
+  const char *test_desc_4 =
+    "So we glanced back simultaneously, it would appear; though no doubt the"
+    "incipient motion of one prompted the imitation of the other. As we did "
+    "so we flashed both torches full strength at the momentarily thinned mis"
+    "t; either from sheer primitive anxiety to see all we could, or in a les"
+    "s primitive but equally unconscious effort to dazzle the entity before "
+    "we dimmed our light and dodged among the penguins of the labyrinth cent"
+    "er ahead. Unhappy act! Not Orpheus himself, or Lot's wife, paid much mo"
+    "re dearly for a backward glance. And again came that shocking, wide-ran"
+    "ged piping - \"Tekeli-li! Tekeli-li!\"";
+  uint8_t test_desc_4_hash[DIGEST256_LEN];
+  char test_desc_4_hash_str[HEX_DIGEST256_LEN+1];
+  (void)data;
+
+  /*
+   * Set up options mock so we can force a tiny FIFO size and generate
+   * cleanups.
+   */
+  mock_options = malloc(sizeof(or_options_t));
+  reset_options(mock_options, &mock_get_options_calls);
+  mock_options->MaxUnparseableDescSizeToLog = 1536;
+  MOCK(get_options, mock_get_options);
+  MOCK(check_private_dir, mock_check_private_dir);
+  MOCK(options_get_datadir_fname2_suffix,
+       mock_get_datadir_fname);
+
+  /*
+   * Set up unlink and write mocks
+   */
+  MOCK(tor_unlink, mock_unlink);
+  mock_unlink_reset();
+  MOCK(write_str_to_file, mock_write_str_to_file);
+  mock_write_str_to_file_reset();
+
+  /*
+   * Compute hashes we'll need to recognize which descriptor is which
+   */
+  crypto_digest256((char *)test_desc_1_hash, test_desc_1,
+                   strlen(test_desc_1), DIGEST_SHA256);
+  base16_encode(test_desc_1_hash_str, sizeof(test_desc_1_hash_str),
+                (const char *)test_desc_1_hash,
+                sizeof(test_desc_1_hash));
+  crypto_digest256((char *)test_desc_2_hash, test_desc_2,
+                   strlen(test_desc_2), DIGEST_SHA256);
+  base16_encode(test_desc_2_hash_str, sizeof(test_desc_2_hash_str),
+                (const char *)test_desc_2_hash,
+                sizeof(test_desc_2_hash));
+  crypto_digest256((char *)test_desc_3_hash, test_desc_3,
+                   strlen(test_desc_3), DIGEST_SHA256);
+  base16_encode(test_desc_3_hash_str, sizeof(test_desc_3_hash_str),
+                (const char *)test_desc_3_hash,
+                sizeof(test_desc_3_hash));
+  crypto_digest256((char *)test_desc_4_hash, test_desc_4,
+                   strlen(test_desc_4), DIGEST_SHA256);
+  base16_encode(test_desc_4_hash_str, sizeof(test_desc_4_hash_str),
+                (const char *)test_desc_4_hash,
+                sizeof(test_desc_4_hash));
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * (1) Fire off dump_desc() once; these descriptors should all be safely
+   * smaller than configured FIFO size.
+   */
+
+  dump_desc(test_desc_1, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_1));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_1_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (2) Fire off dump_desc() twice; this still should trigger no cleanup.
+   */
+
+  /* First time */
+  dump_desc(test_desc_2, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_2_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_3, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2) + strlen(test_desc_3));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_3_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (3) Three calls to dump_desc cause a FIFO cleanup
+   */
+
+  /* First time */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_1, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_4) + strlen(test_desc_1));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_1_hash, DIGEST_SHA256);
+
+  /* Third time - we should unlink the dump of test_desc_4 here */
+  dump_desc(test_desc_2, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_1) + strlen(test_desc_2));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 1);
+  tt_int_op(write_str_count, ==, 3);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_2_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (4) But repeating one (A B B) doesn't overflow and cleanup
+   */
+
+  /* First time */
+  dump_desc(test_desc_3, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_3_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3) + strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /* Third time */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3) + strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (5) Same for the (A B A) repetition
+   */
+
+  /* First time */
+  dump_desc(test_desc_1, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_1));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_1_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_2, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_1) + strlen(test_desc_2));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_2_hash, DIGEST_SHA256);
+
+  /* Third time */
+  dump_desc(test_desc_1, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_1) + strlen(test_desc_2));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_2_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (6) (A B B C) triggering overflow on C causes A, not B to be unlinked
+   */
+
+  /* First time */
+  dump_desc(test_desc_3, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_3_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3) + strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /* Third time */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_3) + strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /* Fourth time - we should unlink the dump of test_desc_3 here */
+  dump_desc(test_desc_1, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_4) + strlen(test_desc_1));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 1);
+  tt_int_op(write_str_count, ==, 3);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_1_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+  /*
+   * (7) (A B A C) triggering overflow on C causes B, not A to be unlinked
+   */
+
+  /* First time */
+  dump_desc(test_desc_2, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 1);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 1);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_2_hash, DIGEST_SHA256);
+
+  /* Second time */
+  dump_desc(test_desc_3, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2) + strlen(test_desc_3));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_3_hash, DIGEST_SHA256);
+
+  /* Third time */
+  dump_desc(test_desc_2, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2) + strlen(test_desc_3));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 2);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_3_hash, DIGEST_SHA256);
+
+  /* Fourth time - we should unlink the dump of test_desc_3 here */
+  dump_desc(test_desc_4, test_desc_type);
+
+  /*
+   * Assert things about the FIFO state
+   */
+  tt_int_op(len_descs_dumped, ==, strlen(test_desc_2) + strlen(test_desc_4));
+  tt_assert(descs_dumped != NULL && smartlist_len(descs_dumped) == 2);
+
+  /*
+   * Assert things about the mocks
+   */
+  tt_int_op(unlinked_count, ==, 1);
+  tt_int_op(write_str_count, ==, 3);
+  tt_mem_op(last_write_str_hash, OP_EQ, test_desc_4_hash, DIGEST_SHA256);
+
+  /*
+   * Reset the FIFO and check its state
+   */
+  dump_desc_fifo_cleanup();
+  tt_int_op(len_descs_dumped, ==, 0);
+  tt_assert(descs_dumped == NULL || smartlist_len(descs_dumped) == 0);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  mock_write_str_to_file_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  tt_int_op(write_str_count, ==, 0);
+
+ done:
+
+  /* Clean up the fifo */
+  dump_desc_fifo_cleanup();
+
+  /* Remove mocks */
+  UNMOCK(tor_unlink);
+  mock_unlink_reset();
+  UNMOCK(write_str_to_file);
+  mock_write_str_to_file_reset();
+  UNMOCK(options_get_datadir_fname2_suffix);
+  UNMOCK(check_private_dir);
+  UNMOCK(get_options);
+  free(mock_options);
+  mock_options = NULL;
+
+  return;
+}
+
+/* Variables for reset_read_file_to_str_mock() */
+
+static char *expected_filename = NULL;
+static char *file_content = NULL;
+static size_t file_content_len = 0;
+static struct stat file_stat;
+static int read_count = 0, read_call_count = 0;
+
+static void
+reset_read_file_to_str_mock(void)
+{
+  tor_free(expected_filename);
+  tor_free(file_content);
+  file_content_len = 0;
+  memset(&file_stat, 0, sizeof(file_stat));
+  read_count = 0;
+  read_call_count = 0;
+}
+
+static char *
+read_file_to_str_mock(const char *filename, int flags,
+                      struct stat *stat_out) {
+  char *result = NULL;
+
+  /* Insist we got a filename */
+  tt_assert(filename != NULL);
+
+  /* We ignore flags */
+  (void)flags;
+
+  /* Bump the call count */
+  ++read_call_count;
+
+  if (expected_filename != NULL &&
+      file_content != NULL &&
+      strcmp(filename, expected_filename) == 0) {
+    /* You asked for it, you got it */
+
+    /*
+     * This is the same behavior as the real read_file_to_str();
+     * if there's a NUL, the real size ends up in stat_out.
+     */
+    result = tor_malloc(file_content_len + 1);
+    if (file_content_len > 0) {
+      memcpy(result, file_content, file_content_len);
+    }
+    result[file_content_len] = '\0';
+
+    /* Do we need to set up stat_out? */
+    if (stat_out != NULL) {
+      memcpy(stat_out, &file_stat, sizeof(file_stat));
+      /* We always return the correct length here */
+      stat_out->st_size = file_content_len;
+    }
+
+    /* Wooo, we have a return value - bump the counter */
+    ++read_count;
+  }
+  /* else no match, return NULL */
+
+ done:
+  return result;
+}
+
+/* This one tests dump_desc_populate_one_file() */
+static void
+test_dir_populate_dump_desc_fifo(void *data)
+{
+  const char *dirname = "foo";
+  const char *fname = NULL;
+  dumped_desc_t *ent;
+
+  (void)data;
+
+  /*
+   * Set up unlink and read_file_to_str mocks
+   */
+  MOCK(tor_unlink, mock_unlink);
+  mock_unlink_reset();
+  MOCK(read_file_to_str, read_file_to_str_mock);
+  reset_read_file_to_str_mock();
+
+  /* Check state of unlink mock */
+  tt_int_op(unlinked_count, ==, 0);
+
+  /* Some cases that should fail before trying to read the file */
+  ent = dump_desc_populate_one_file(dirname, "bar");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 1);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  ent = dump_desc_populate_one_file(dirname, "unparseable-desc");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 2);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  ent = dump_desc_populate_one_file(dirname, "unparseable-desc.baz");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 3);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  ent = dump_desc_populate_one_file(
+      dirname,
+      "unparseable-desc.08AE85E90461F59E");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 4);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  ent = dump_desc_populate_one_file(
+      dirname,
+      "unparseable-desc.08AE85E90461F59EDF0981323F3A70D02B55AB54B44B04F"
+      "287D72F7B72F242E85C8CB0EDA8854A99");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 5);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  /* This is a correct-length digest but base16_decode() will fail */
+  ent = dump_desc_populate_one_file(
+      dirname,
+      "unparseable-desc.68219B8BGE64B705A6FFC728C069DC596216D60A7D7520C"
+      "D5ECE250D912E686B");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 6);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 0);
+
+  /* This one has a correctly formed filename and should try reading */
+
+  /* Read fails */
+  ent = dump_desc_populate_one_file(
+      dirname,
+      "unparseable-desc.DF0981323F3A70D02B55AB54B44B04F287D72F7B72F242E"
+      "85C8CB0EDA8854A99");
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 7);
+  tt_int_op(read_count, ==, 0);
+  tt_int_op(read_call_count, ==, 1);
+
+  /* This read will succeed but the digest won't match the file content */
+  fname =
+    "unparseable-desc."
+    "DF0981323F3A70D02B55AB54B44B04F287D72F7B72F242E85C8CB0EDA8854A99";
+  tor_asprintf(&expected_filename, "%s/%s", dirname, fname);
+  file_content = tor_strdup("hanc culpam maiorem an illam dicam?");
+  file_content_len = strlen(file_content);
+  file_stat.st_mtime = 123456;
+  ent = dump_desc_populate_one_file(dirname, fname);
+  tt_assert(ent == NULL);
+  tt_int_op(unlinked_count, ==, 8);
+  tt_int_op(read_count, ==, 1);
+  tt_int_op(read_call_count, ==, 2);
+  tor_free(expected_filename);
+
+  /* This one will match */
+  fname =
+    "unparseable-desc."
+    "0786C7173447B7FB033FFCA2FC47C3CF71C30DD47CA8236D3FC7FF35853271C6";
+  tor_asprintf(&expected_filename, "%s/%s", dirname, fname);
+  file_content = tor_strdup("hanc culpam maiorem an illam dicam?");
+  file_content_len = strlen(file_content);
+  file_stat.st_mtime = 789012;
+  ent = dump_desc_populate_one_file(dirname, fname);
+  tt_assert(ent != NULL);
+  tt_int_op(unlinked_count, ==, 8);
+  tt_int_op(read_count, ==, 2);
+  tt_int_op(read_call_count, ==, 3);
+  tt_str_op(ent->filename, OP_EQ, expected_filename);
+  tt_int_op(ent->len, ==, file_content_len);
+  tt_int_op(ent->when, ==, file_stat.st_mtime);
+  tor_free(ent->filename);
+  tor_free(ent);
+  tor_free(expected_filename);
+
+  /*
+   * Reset the mocks and check their state
+   */
+  mock_unlink_reset();
+  tt_int_op(unlinked_count, ==, 0);
+  reset_read_file_to_str_mock();
+  tt_int_op(read_count, ==, 0);
+
+ done:
+
+  UNMOCK(tor_unlink);
+  mock_unlink_reset();
+  UNMOCK(read_file_to_str);
+  reset_read_file_to_str_mock();
+
+  return;
+}
+
+static smartlist_t *
+listdir_mock(const char *dname)
+{
+  smartlist_t *l;
+
+  /* Ignore the name, always return this list */
+  (void)dname;
+
+  l = smartlist_new();
+  smartlist_add(l, tor_strdup("foo"));
+  smartlist_add(l, tor_strdup("bar"));
+  smartlist_add(l, tor_strdup("baz"));
+
+  return l;
+}
+
+static dumped_desc_t *
+pop_one_mock(const char *dirname, const char *f)
+{
+  dumped_desc_t *ent = NULL;
+
+  if (dirname != NULL && strcmp(dirname, "d") == 0) {
+    if (f != NULL && strcmp(f, "foo") == 0) {
+      ent = tor_malloc_zero(sizeof(*ent));
+      ent->filename = strdup("d/foo");
+      ent->len = 123;
+      ent->digest_sha256[0] = 1;
+      ent->when = 1024;
+    } else if (f != NULL && strcmp(f, "bar") == 0) {
+      ent = tor_malloc_zero(sizeof(*ent));
+      ent->filename = strdup("d/bar");
+      ent->len = 456;
+      ent->digest_sha256[0] = 2;
+      /*
+       * Note that the timestamps are in a different order than
+       * listdir_mock() returns; we're testing the sort order.
+       */
+      ent->when = 512;
+    } else if (f != NULL && strcmp(f, "baz") == 0) {
+      ent = tor_malloc_zero(sizeof(*ent));
+      ent->filename = strdup("d/baz");
+      ent->len = 789;
+      ent->digest_sha256[0] = 3;
+      ent->when = 768;
+    }
+  }
+
+  return ent;
+}
+
+/* This one tests dump_desc_populate_fifo_from_directory() */
+static void
+test_dir_populate_dump_desc_fifo_2(void *data)
+{
+  dumped_desc_t *ent = NULL;
+
+  (void)data;
+
+  /* Set up the mocks */
+  MOCK(tor_listdir, listdir_mock);
+  MOCK(dump_desc_populate_one_file, pop_one_mock);
+
+  /* Run dump_desc_populate_fifo_from_directory() */
+  descs_dumped = NULL;
+  len_descs_dumped = 0;
+  dump_desc_populate_fifo_from_directory("d");
+  tt_assert(descs_dumped != NULL);
+  tt_int_op(smartlist_len(descs_dumped), OP_EQ, 3);
+  tt_int_op(len_descs_dumped, OP_EQ, 1368);
+  ent = smartlist_get(descs_dumped, 0);
+  tt_str_op(ent->filename, OP_EQ, "d/bar");
+  tt_int_op(ent->len, OP_EQ, 456);
+  tt_int_op(ent->when, OP_EQ, 512);
+  ent = smartlist_get(descs_dumped, 1);
+  tt_str_op(ent->filename, OP_EQ, "d/baz");
+  tt_int_op(ent->len, OP_EQ, 789);
+  tt_int_op(ent->when, OP_EQ, 768);
+  ent = smartlist_get(descs_dumped, 2);
+  tt_str_op(ent->filename, OP_EQ, "d/foo");
+  tt_int_op(ent->len, OP_EQ, 123);
+  tt_int_op(ent->when, OP_EQ, 1024);
+
+ done:
+  dump_desc_fifo_cleanup();
+
+  UNMOCK(dump_desc_populate_one_file);
+  UNMOCK(tor_listdir);
+
+  return;
+}
+
 static int mock_networkstatus_consensus_is_bootstrapping_value = 0;
 static int
 mock_networkstatus_consensus_is_bootstrapping(time_t now)
@@ -4310,6 +5297,9 @@ struct testcase_t dir_tests[] = {
   DIR(should_not_init_request_to_dir_auths_without_v3_info, 0),
   DIR(should_init_request_to_dir_auths, 0),
   DIR(choose_compression_level, 0),
+  DIR(dump_unparseable_descriptors, 0),
+  DIR(populate_dump_desc_fifo, 0),
+  DIR(populate_dump_desc_fifo_2, 0),
   DIR_ARG(find_dl_schedule, TT_FORK, "bf"),
   DIR_ARG(find_dl_schedule, TT_FORK, "ba"),
   DIR_ARG(find_dl_schedule, TT_FORK, "cf"),