From 1cde3e2776dc879ce7ca876e0616d8d10d6f1702 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 17 Jun 2016 22:35:58 +0000 Subject: Add multiple descriptor dump support for dump_desc() in routerparse.c; fixes bug 18322 --- src/or/config.c | 1 + src/or/main.c | 1 + src/or/or.h | 5 ++ src/or/routerparse.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/or/routerparse.h | 2 + 5 files changed, 211 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/or/config.c b/src/or/config.c index cdd4f10e92..275be287c9 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -212,6 +212,7 @@ static config_var_t option_vars_[] = { V(CountPrivateBandwidth, BOOL, "0"), V(DataDirectory, FILENAME, NULL), V(DataDirectoryGroupReadable, BOOL, "0"), + V(DetailedLogForUnparseableDescriptors, MEMUNIT, "0 bytes"), V(DisableNetwork, BOOL, "0"), V(DirAllowPrivateAddresses, BOOL, "0"), V(TestingAuthDirTimeToLearnReachability, INTERVAL, "30 minutes"), diff --git a/src/or/main.c b/src/or/main.c index 4de2e70a1d..32915706ef 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -3146,6 +3146,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..364699fdf0 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; + + /** If > 0, maximum total size of unparseable descriptors to log + * during the lifetime of this Tor process. + */ + uint64_t DetailedLogForUnparseableDescriptors; } 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..5ad65491a8 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -587,6 +587,99 @@ static int check_signature_token(const char *digest, /** Last time we dumped a descriptor to disk. */ static time_t last_desc_dumped = 0; +/** List of dumped descriptors for FIFO cleanup purposes */ +static smartlist_t *descs_dumped = NULL; +/** Total size of dumped descriptors for FIFO cleanup */ +static size_t len_descs_dumped = 0; + +typedef struct { + char *filename; + size_t len; + time_t timestamp; +} dumped_desc_t; + +/** 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. */ +static void +dump_desc_fifo_add_and_clean(char *filename, size_t len, time_t now) +{ + dumped_desc_t *ent = NULL, *tmp; + size_t max_len; + + 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->timestamp = now; + + /* Do we need to do some cleanup? */ + if (get_options()->DetailedLogForUnparseableDescriptors > 0) { + max_len = get_options()->DetailedLogForUnparseableDescriptors; + /* Iterate over the list until we've freed enough space */ + while (len_descs_dumped + len > max_len && + 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 */ + 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; +} + +/** 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; + } +} /** For debugging purposes, dump unparseable descriptor *desc of * type *type to file $DATADIR/unparseable-desc. Do not write more @@ -598,19 +691,112 @@ 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); + /* Are we dumping this one to a file? */ + int dumping_this_one; + /* + * Are we including the hash in the filename and using the + * FIFO mechanism? + */ + int dumping_by_hash; + /* Emit an "Unable to parse descriptor..." prefix? */ + int emit_prefix; + 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; + /* Expected length of file */ + size_t filelen; + /* File content */ + char *content; + + /* 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)); + + if (get_options()->DetailedLogForUnparseableDescriptors > 0) { + /* Okay, we're logging to a fresh file named by hash for each one */ + dumping_by_hash = 1; + dumping_this_one = 1; + /* + * Detailed logging mechanism will mention type and hash in the main log; + * don't clutter up the files with anything but the exact dump. + */ + emit_prefix = 0; + tor_asprintf(&debugfile_base, "unparseable-desc.%s", digest_sha256_hex); + debugfile = get_datadir_fname(debugfile_base); + } else if (!last_desc_dumped || last_desc_dumped + 60 < now) { + /* Simple mechanism, check so we don't dump too often */ + dumping_by_hash = 0; + dumping_this_one = 1; + emit_prefix = 1; + debugfile_base = tor_strdup("unparseable-desc"); + debugfile = get_datadir_fname(debugfile_base); + } else { + /* No logging of the full descriptor this time */ + dumping_by_hash = 0; + dumping_this_one = 0; + emit_prefix = 1; + debugfile_base = NULL; + debugfile = NULL; + } + + if (dumping_this_one) { + if (emit_prefix) filelen = 50 + strlen(type) + len; + else filelen = len; + + /* Get content pointing at what we're writing */ + if (emit_prefix) { + content = tor_malloc_zero(filelen); + tor_snprintf(content, filelen, "Unable to parse descriptor of type " + "%s:\n%s", type, desc); + } else { + content = tor_strdup(desc); + } + + /* Write it, and tell the main log about it */ 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); + 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); + + /* Free our in-memory copy if necessary */ tor_free(content); - tor_free(debugfile); + last_desc_dumped = now; + + /* Give it to the FIFO and clean up as needed, if we're using one */ + if (dumping_by_hash) { + dump_desc_fifo_add_and_clean(debugfile, filelen, now); + /* Since we handed ownership over, don't free debugfile later */ + debugfile = NULL; + } + } 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 since we just dumped one %u seconds " + "ago.", + type, digest_sha256_hex, (unsigned long)len, + (unsigned int)(now - last_desc_dumped)); } + + if (debugfile_base != NULL) tor_free(debugfile_base); + if (debugfile != NULL) tor_free(debugfile); + + err: + return; } /** Set digest to the SHA-1 digest of the hash of the directory in @@ -5468,3 +5654,10 @@ rend_parse_client_keys(strmap_t *parsed_clients, const char *ckstr) return result; } +/** 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..69160d1227 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -85,6 +85,8 @@ 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_free_all(void); + #ifdef ROUTERPARSE_PRIVATE STATIC int routerstatus_parse_guardfraction(const char *guardfraction_str, networkstatus_t *vote, -- cgit v1.2.3-54-g00ecf From 726dc9acf599567bf2ed1786fc2af785f15cac25 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 05:20:46 +0000 Subject: Remove old unparseable descriptor logging mechanism, add bump-to-head-of-queue for repeated unparseable descriptors, rename config variable --- src/or/config.c | 2 +- src/or/or.h | 6 +- src/or/routerparse.c | 231 ++++++++++++++++++++++++++------------------------- 3 files changed, 123 insertions(+), 116 deletions(-) (limited to 'src') diff --git a/src/or/config.c b/src/or/config.c index 275be287c9..b00f8a9244 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -212,7 +212,6 @@ static config_var_t option_vars_[] = { V(CountPrivateBandwidth, BOOL, "0"), V(DataDirectory, FILENAME, NULL), V(DataDirectoryGroupReadable, BOOL, "0"), - V(DetailedLogForUnparseableDescriptors, MEMUNIT, "0 bytes"), V(DisableNetwork, BOOL, "0"), V(DirAllowPrivateAddresses, BOOL, "0"), V(TestingAuthDirTimeToLearnReachability, INTERVAL, "30 minutes"), @@ -326,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"), diff --git a/src/or/or.h b/src/or/or.h index 364699fdf0..a1a0810e4b 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4499,10 +4499,10 @@ typedef struct { /** Autobool: Do we try to retain capabilities if we can? */ int KeepBindCapabilities; - /** If > 0, maximum total size of unparseable descriptors to log - * during the lifetime of this Tor process. + /** Maximum total size of unparseable descriptors to log during the + * lifetime of this Tor process. */ - uint64_t DetailedLogForUnparseableDescriptors; + 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 5ad65491a8..1d227fdc70 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -585,28 +585,40 @@ 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 size_t len_descs_dumped = 0; +/* + * One entry in the list of dumped descriptors; filename dumped to, length + * and SHA-256. + */ + typedef struct { char *filename; size_t len; - time_t timestamp; + uint8_t digest_sha256[DIGEST256_LEN]; } dumped_desc_t; /** 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. */ + * 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, size_t len, time_t now) +dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256, + size_t len) { dumped_desc_t *ent = NULL, *tmp; size_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); @@ -618,43 +630,45 @@ dump_desc_fifo_add_and_clean(char *filename, size_t len, time_t now) ent = tor_malloc_zero(sizeof(*ent)); ent->filename = filename; ent->len = len; - ent->timestamp = now; + memcpy(ent->digest_sha256, digest_sha256, DIGEST256_LEN); /* Do we need to do some cleanup? */ - if (get_options()->DetailedLogForUnparseableDescriptors > 0) { - max_len = get_options()->DetailedLogForUnparseableDescriptors; - /* Iterate over the list until we've freed enough space */ - while (len_descs_dumped + len > max_len && - smartlist_len(descs_dumped) > 0) { - /* Get the oldest thing on the list */ - tmp = (dumped_desc_t *)(smartlist_get(descs_dumped, 0)); + max_len = get_options()->MaxUnparseableDescSizeToLog; + /* Iterate over the list until we've freed enough space */ + while (len_descs_dumped + len > max_len && + 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 */ + 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 { /* - * Check if it matches the filename we just added, so we don't - * delete something we just emitted if we get repeated identical - * descriptors. + * Don't delete, but do adjust the counter since we will bump it + * later */ - if (strcmp(tmp->filename, filename) != 0) { - /* Delete it and adjust the length counter */ - 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); + 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 */ @@ -662,6 +676,43 @@ dump_desc_fifo_add_and_clean(char *filename, size_t len, time_t now) 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) { + /* 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 @@ -688,28 +739,14 @@ dump_desc_fifo_cleanup(void) static void dump_desc(const char *desc, const char *type) { - time_t now = time(NULL); tor_assert(desc); tor_assert(type); - /* Are we dumping this one to a file? */ - int dumping_this_one; - /* - * Are we including the hash in the filename and using the - * FIFO mechanism? - */ - int dumping_by_hash; - /* Emit an "Unable to parse descriptor..." prefix? */ - int emit_prefix; 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; - /* Expected length of file */ - size_t filelen; - /* File content */ - char *content; /* Get the hash for logging purposes anyway */ len = strlen(desc); @@ -724,76 +761,46 @@ dump_desc(const char *desc, const char *type) base16_encode(digest_sha256_hex, sizeof(digest_sha256_hex), (const char *)digest_sha256, sizeof(digest_sha256)); - if (get_options()->DetailedLogForUnparseableDescriptors > 0) { - /* Okay, we're logging to a fresh file named by hash for each one */ - dumping_by_hash = 1; - dumping_this_one = 1; - /* - * Detailed logging mechanism will mention type and hash in the main log; - * don't clutter up the files with anything but the exact dump. - */ - emit_prefix = 0; - tor_asprintf(&debugfile_base, "unparseable-desc.%s", digest_sha256_hex); - debugfile = get_datadir_fname(debugfile_base); - } else if (!last_desc_dumped || last_desc_dumped + 60 < now) { - /* Simple mechanism, check so we don't dump too often */ - dumping_by_hash = 0; - dumping_this_one = 1; - emit_prefix = 1; - debugfile_base = tor_strdup("unparseable-desc"); - debugfile = get_datadir_fname(debugfile_base); - } else { - /* No logging of the full descriptor this time */ - dumping_by_hash = 0; - dumping_this_one = 0; - emit_prefix = 1; - debugfile_base = NULL; - debugfile = NULL; - } - - if (dumping_this_one) { - if (emit_prefix) filelen = 50 + strlen(type) + len; - else filelen = len; - - /* Get content pointing at what we're writing */ - if (emit_prefix) { - content = tor_malloc_zero(filelen); - tor_snprintf(content, filelen, "Unable to parse descriptor of type " - "%s:\n%s", type, desc); - } else { - content = tor_strdup(desc); - } - - /* Write it, and tell the main log about it */ - write_str_to_file(debugfile, content, 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); - - /* Free our in-memory copy if necessary */ - tor_free(content); - - last_desc_dumped = now; - - /* Give it to the FIFO and clean up as needed, if we're using one */ - if (dumping_by_hash) { - dump_desc_fifo_add_and_clean(debugfile, filelen, now); + /* + * 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, "unparseable-desc.%s", digest_sha256_hex); + debugfile = get_datadir_fname(debugfile_base); + + if (len <= get_options()->MaxUnparseableDescSizeToLog) { + if (!dump_desc_fifo_bump_hash(digest_sha256)) { + /* 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 { + /* 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 since we just dumped one %u seconds " - "ago.", - type, digest_sha256_hex, (unsigned long)len, - (unsigned int)(now - last_desc_dumped)); + "%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 */ } - if (debugfile_base != NULL) tor_free(debugfile_base); - if (debugfile != NULL) tor_free(debugfile); + tor_free(debugfile_base); + tor_free(debugfile); err: return; -- cgit v1.2.3-54-g00ecf From 17ed2fed68ed18480976748e3f407bf6952ab72c Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 06:26:44 +0000 Subject: Expose dump_desc() to the test suite and make things it calls mockable --- src/common/util.c | 14 ++++++++++++-- src/common/util.h | 5 ++++- src/or/routerparse.c | 4 ++-- src/or/routerparse.h | 1 + 4 files changed, 19 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/common/util.c b/src/common/util.c index 4b6df81b7d..d4553c3942 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2013,6 +2013,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, @@ -2306,8 +2316,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')) { diff --git a/src/common/util.h b/src/common/util.h index 7cb33dc680..fd9b983d10 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; @@ -338,7 +340,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)); diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 1d227fdc70..eacf9aa916 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -646,7 +646,7 @@ dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256, */ if (strcmp(tmp->filename, filename) != 0) { /* Delete it and adjust the length counter */ - unlink(tmp->filename); + tor_unlink(tmp->filename); tor_assert(len_descs_dumped >= tmp->len); len_descs_dumped -= tmp->len; log_info(LD_DIR, @@ -736,7 +736,7 @@ dump_desc_fifo_cleanup(void) * type *type 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) { tor_assert(desc); diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 69160d1227..fe32fb7348 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -92,6 +92,7 @@ STATIC int routerstatus_parse_guardfraction(const char *guardfraction_str, networkstatus_t *vote, vote_routerstatus_t *vote_rs, routerstatus_t *rs); +STATIC void dump_desc(const char *desc, const char *type); #endif #define ED_DESC_SIGNATURE_PREFIX "Tor router descriptor signature v1" -- cgit v1.2.3-54-g00ecf From 4e4a760491ef162413240b0bb38732da801e818a Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 07:47:25 +0000 Subject: Add extern support for file-scope variables in testsupport.h --- src/common/testsupport.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src') diff --git a/src/common/testsupport.h b/src/common/testsupport.h index 3bb11a7e41..b98d7014c7 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. -- cgit v1.2.3-54-g00ecf From 35fc5879fb15177581284537f5b5286110590555 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 07:47:53 +0000 Subject: Expose a few more dump_desc()-related things to the test suite --- src/or/routerparse.c | 6 +++--- src/or/routerparse.h | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index eacf9aa916..5f1dde4dc9 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -588,9 +588,9 @@ static int check_signature_token(const char *digest, /* Dump mechanism for unparseable descriptors */ /** List of dumped descriptors for FIFO cleanup purposes */ -static smartlist_t *descs_dumped = NULL; +STATIC smartlist_t *descs_dumped = NULL; /** Total size of dumped descriptors for FIFO cleanup */ -static size_t len_descs_dumped = 0; +STATIC size_t len_descs_dumped = 0; /* * One entry in the list of dumped descriptors; filename dumped to, length @@ -715,7 +715,7 @@ dump_desc_fifo_bump_hash(const uint8_t *digest_sha256) /** Clean up on exit; just memory, leave the dumps behind */ -static void +STATIC void dump_desc_fifo_cleanup(void) { if (descs_dumped) { diff --git a/src/or/routerparse.h b/src/or/routerparse.h index fe32fb7348..2b18143b06 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -88,11 +88,14 @@ int rend_parse_client_keys(strmap_t *parsed_clients, const char *str); void routerparse_free_all(void); #ifdef ROUTERPARSE_PRIVATE +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); 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" -- cgit v1.2.3-54-g00ecf From 2a17b93cc45e62c589de4f413ab0314f9bf72193 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 08:31:45 +0000 Subject: Make options_get_datadir_fname2_suffix() mockable --- src/or/config.c | 8 ++++---- src/or/config.h | 8 +++++--- 2 files changed, 9 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/or/config.c b/src/or/config.c index b00f8a9244..18ee9790f6 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -7229,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 -- cgit v1.2.3-54-g00ecf From 824ee581b02d83d331506ab699ba3110f43595cb Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 25 Jun 2016 08:32:16 +0000 Subject: Add dir/dump_unparseable_descriptors unit test --- src/test/test_dir.c | 675 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 675 insertions(+) (limited to 'src') diff --git a/src/test/test_dir.c b/src/test/test_dir.c index b8dcab39d5..e18ed4204b 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,679 @@ test_dir_choose_compression_level(void* data) done: ; } +/* + * 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_fname(), since it's all + * we implement here. + */ + tt_assert(options != NULL); + tt_assert(sub1 != NULL); + tt_assert(sub2 == NULL); + tt_assert(suffix == NULL); + + /* Just duplicate the basename and return it for this mock */ + 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(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(get_options); + free(mock_options); + mock_options = NULL; + + return; +} + static int mock_networkstatus_consensus_is_bootstrapping_value = 0; static int mock_networkstatus_consensus_is_bootstrapping(time_t now) @@ -4310,6 +4984,7 @@ 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_ARG(find_dl_schedule, TT_FORK, "bf"), DIR_ARG(find_dl_schedule, TT_FORK, "ba"), DIR_ARG(find_dl_schedule, TT_FORK, "cf"), -- cgit v1.2.3-54-g00ecf From 603f483092778786e29944acf71a608bfa21650b Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Wed, 29 Jun 2016 22:40:28 +0000 Subject: Use uint64_t for total length of dumped descriptors, nad be careful about overflows in the loop in dump_desc_fifo_add_and_clean() --- src/or/routerparse.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 5f1dde4dc9..afdfcbd403 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -590,7 +590,7 @@ static int check_signature_token(const char *digest, /** List of dumped descriptors for FIFO cleanup purposes */ STATIC smartlist_t *descs_dumped = NULL; /** Total size of dumped descriptors for FIFO cleanup */ -STATIC size_t len_descs_dumped = 0; +STATIC uint64_t len_descs_dumped = 0; /* * One entry in the list of dumped descriptors; filename dumped to, length @@ -614,7 +614,7 @@ dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256, size_t len) { dumped_desc_t *ent = NULL, *tmp; - size_t max_len; + uint64_t max_len; tor_assert(filename != NULL); tor_assert(digest_sha256 != NULL); @@ -635,7 +635,7 @@ dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256, /* Do we need to do some cleanup? */ max_len = get_options()->MaxUnparseableDescSizeToLog; /* Iterate over the list until we've freed enough space */ - while (len_descs_dumped + len > max_len && + 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)); -- cgit v1.2.3-54-g00ecf From dc37546cff2f025613ef142e74ad4db1c7d99ade Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Wed, 29 Jun 2016 22:47:41 +0000 Subject: Add sandbox_is_active() check to dump_desc() --- src/or/routerparse.c | 55 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index afdfcbd403..93b90cc28d 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 @@ -768,35 +769,49 @@ dump_desc(const char *desc, const char *type) tor_asprintf(&debugfile_base, "unparseable-desc.%s", digest_sha256_hex); debugfile = get_datadir_fname(debugfile_base); - if (len <= get_options()->MaxUnparseableDescSizeToLog) { - if (!dump_desc_fifo_bump_hash(digest_sha256)) { - /* 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; + if (!sandbox_is_active()) { + if (len <= get_options()->MaxUnparseableDescSizeToLog) { + if (!dump_desc_fifo_bump_hash(digest_sha256)) { + /* 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 { + /* 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 { - /* We already had one with this hash dumped */ + /* 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 one with that hash " - "has already been dumped.", + "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 { - /* Just log that it happened without dumping */ + /* + * 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 it exceeds maximum log size " - "all by itself.", + "Unable to parse descriptor of type %s with hash %s and " + "length %lu. Descriptor not dumped because the sandbox is " + "active", type, digest_sha256_hex, (unsigned long)len); - /* We do have to free debugfile in this case */ } tor_free(debugfile_base); -- cgit v1.2.3-54-g00ecf From 38cced90ef62aff04477d1b814ab2ee3b7776638 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 00:39:29 +0000 Subject: Move unparseable descriptor dumps into subdirectory of DataDir --- src/common/util.c | 6 +-- src/common/util.h | 5 +- src/or/main.c | 3 ++ src/or/routerparse.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++----- src/or/routerparse.h | 1 + src/test/test_dir.c | 34 +++++++++++-- 6 files changed, 162 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/common/util.c b/src/common/util.c index d4553c3942..d60380cb9b 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2086,9 +2086,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; diff --git a/src/common/util.h b/src/common/util.h index fd9b983d10..70483bbc24 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -326,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) diff --git a/src/or/main.c b/src/or/main.c index 32915706ef..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; } diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 93b90cc28d..75c09d2bb4 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -592,6 +592,11 @@ static int check_signature_token(const char *digest, 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" /* * One entry in the list of dumped descriptors; filename dumped to, length @@ -604,6 +609,88 @@ typedef struct { uint8_t digest_sha256[DIGEST256_LEN]; } dumped_desc_t; +/** 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; + } + + 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 @@ -767,21 +854,35 @@ dump_desc(const char *desc, const char *type) * with anything but the exact dump. */ tor_asprintf(&debugfile_base, "unparseable-desc.%s", digest_sha256_hex); - debugfile = get_datadir_fname(debugfile_base); + debugfile = get_datadir_fname2(DESC_DUMP_DATADIR_SUBDIR, debugfile_base); if (!sandbox_is_active()) { if (len <= get_options()->MaxUnparseableDescSizeToLog) { if (!dump_desc_fifo_bump_hash(digest_sha256)) { - /* 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; + /* 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, @@ -5676,6 +5777,16 @@ 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) +{ + dump_desc_init(); +} + /** Clean up all data structures used by routerparse.c at exit */ void routerparse_free_all(void) diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 2b18143b06..a96146acf2 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -85,6 +85,7 @@ 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 diff --git a/src/test/test_dir.c b/src/test/test_dir.c index e18ed4204b..b4fc615cb9 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -4102,6 +4102,22 @@ 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 @@ -4118,16 +4134,24 @@ mock_get_datadir_fname(const or_options_t *options, char *rv = NULL; /* - * Assert we were called like get_datadir_fname(), since it's all - * we implement here. + * 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); - tt_assert(sub2 == 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 */ - rv = strdup(sub1); + if (sub2) { + /* If we have sub2, it's the basename, otherwise sub1 */ + rv = strdup(sub2); + } else { + rv = strdup(sub1); + } done: return rv; @@ -4262,6 +4286,7 @@ test_dir_dump_unparseable_descriptors(void *data) 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); @@ -4768,6 +4793,7 @@ test_dir_dump_unparseable_descriptors(void *data) 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; -- cgit v1.2.3-54-g00ecf From 421cf21b3cc7c0d4a9c64eb992b991019103a7f3 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 04:36:23 +0000 Subject: Reload unparseable descriptor dump FIFO state from on-disk dumped descriptors at startup --- src/or/routerparse.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 232 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 75c09d2bb4..cedcfe04c7 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -588,6 +588,8 @@ static int check_signature_token(const char *digest, /* Dump mechanism for unparseable descriptors */ +static void dump_desc_populate_fifo_from_directory(const char *dirname); + /** List of dumped descriptors for FIFO cleanup purposes */ STATIC smartlist_t *descs_dumped = NULL; /** Total size of dumped descriptors for FIFO cleanup */ @@ -597,6 +599,7 @@ 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" /* * One entry in the list of dumped descriptors; filename dumped to, length @@ -607,6 +610,7 @@ typedef struct { char *filename; size_t len; uint8_t digest_sha256[DIGEST256_LEN]; + time_t when; } dumped_desc_t; /** Find the dump directory and check if we'll be able to create it */ @@ -660,6 +664,10 @@ dump_desc_init(void) 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); } @@ -718,6 +726,7 @@ dump_desc_fifo_add_and_clean(char *filename, const uint8_t *digest_sha256, 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? */ @@ -790,6 +799,8 @@ dump_desc_fifo_bump_hash(const uint8_t *digest_sha256) } 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); @@ -820,6 +831,225 @@ dump_desc_fifo_cleanup(void) } } +/** 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. + */ +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. */ + + 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 "." 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 *desc of * type *type to file $DATADIR/unparseable-desc. Do not write more * than one descriptor to disk per minute. If there is already such a @@ -853,7 +1083,8 @@ dump_desc(const char *desc, const char *type) * 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, "unparseable-desc.%s", digest_sha256_hex); + tor_asprintf(&debugfile_base, + DESC_DUMP_BASE_FILENAME ".%s", digest_sha256_hex); debugfile = get_datadir_fname2(DESC_DUMP_DATADIR_SUBDIR, debugfile_base); if (!sandbox_is_active()) { -- cgit v1.2.3-54-g00ecf From 2154160a2484d852e4a0b509a5f046001d52fd11 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 05:35:46 +0000 Subject: Add support for mocking functions declared with attributes without causing gcc warnings --- src/common/testsupport.h | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'src') diff --git a/src/common/testsupport.h b/src/common/testsupport.h index b98d7014c7..9ad2ba77e0 100644 --- a/src/common/testsupport.h +++ b/src/common/testsupport.h @@ -62,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); \ @@ -73,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 -- cgit v1.2.3-54-g00ecf From 42f089473a50613ae62875ede386861e91123b78 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 06:13:44 +0000 Subject: Unit test for dump_desc_populate_one_file() --- src/common/util.c | 4 +- src/common/util.h | 5 +- src/or/routerparse.c | 14 +--- src/or/routerparse.h | 14 ++++ src/test/test_dir.c | 196 +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 216 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/common/util.c b/src/common/util.c index d60380cb9b..97837f565d 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2676,8 +2676,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; diff --git a/src/common/util.h b/src/common/util.h index 70483bbc24..157d25a812 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -367,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; diff --git a/src/or/routerparse.c b/src/or/routerparse.c index cedcfe04c7..f3f003eede 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -601,18 +601,6 @@ static int problem_with_dump_desc_dir = 0; #define DESC_DUMP_DATADIR_SUBDIR "unparseable-descs" #define DESC_DUMP_BASE_FILENAME "unparseable-desc" -/* - * One entry in the list of dumped descriptors; filename dumped to, length - * and SHA-256. - */ - -typedef struct { - char *filename; - size_t len; - uint8_t digest_sha256[DIGEST256_LEN]; - time_t when; -} dumped_desc_t; - /** Find the dump directory and check if we'll be able to create it */ static void dump_desc_init(void) @@ -835,7 +823,7 @@ dump_desc_fifo_cleanup(void) * 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. */ -static dumped_desc_t * +STATIC dumped_desc_t * dump_desc_populate_one_file(const char *dirname, const char *f) { dumped_desc_t *ent = NULL; diff --git a/src/or/routerparse.h b/src/or/routerparse.h index a96146acf2..6167e5148b 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -89,12 +89,26 @@ 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); +STATIC dumped_desc_t * dump_desc_populate_one_file(const char *dirname, + const char *f); STATIC void dump_desc(const char *desc, const char *type); STATIC void dump_desc_fifo_cleanup(void); #endif diff --git a/src/test/test_dir.c b/src/test/test_dir.c index b4fc615cb9..2c398e36c3 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -4801,6 +4801,201 @@ test_dir_dump_unparseable_descriptors(void *data) 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; +} + +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 int mock_networkstatus_consensus_is_bootstrapping_value = 0; static int mock_networkstatus_consensus_is_bootstrapping(time_t now) @@ -5011,6 +5206,7 @@ struct testcase_t dir_tests[] = { 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_ARG(find_dl_schedule, TT_FORK, "bf"), DIR_ARG(find_dl_schedule, TT_FORK, "ba"), DIR_ARG(find_dl_schedule, TT_FORK, "cf"), -- cgit v1.2.3-54-g00ecf From f99c9df02b6ac0c7804013bca951ecaf5744c2db Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 06:27:14 +0000 Subject: Make things mockable for dump_desc_populate_fifo_from_directory() unit test --- src/common/util.c | 4 ++-- src/common/util.h | 2 +- src/or/routerparse.c | 4 ++-- src/or/routerparse.h | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/common/util.c b/src/common/util.c index 97837f565d..725e110daa 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3412,8 +3412,8 @@ smartlist_add_vasprintf(struct smartlist_t *sl, const char *pattern, /** Return a new list containing the filenames in the directory dirname. * Return NULL on error or if dirname 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 157d25a812..44f510cef7 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -377,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/routerparse.c b/src/or/routerparse.c index f3f003eede..3436bfb86b 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -823,8 +823,8 @@ dump_desc_fifo_cleanup(void) * 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. */ -STATIC dumped_desc_t * -dump_desc_populate_one_file(const char *dirname, const char *f) +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; diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 6167e5148b..131f158c56 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -107,8 +107,8 @@ STATIC int routerstatus_parse_guardfraction(const char *guardfraction_str, networkstatus_t *vote, vote_routerstatus_t *vote_rs, routerstatus_t *rs); -STATIC dumped_desc_t * dump_desc_populate_one_file(const char *dirname, - const char *f); +MOCK_DECL(STATIC dumped_desc_t *, dump_desc_populate_one_file, + (const char *dirname, const char *f)); STATIC void dump_desc(const char *desc, const char *type); STATIC void dump_desc_fifo_cleanup(void); #endif -- cgit v1.2.3-54-g00ecf From 9580b99dab217cc14b3dab78962bfb3bfd51922a Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 06:59:29 +0000 Subject: Add unit test for dump_desc_populate_fifo_from_directory() --- src/or/routerparse.c | 4 +-- src/or/routerparse.h | 1 + src/test/test_dir.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 3436bfb86b..2260693939 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -588,8 +588,6 @@ static int check_signature_token(const char *digest, /* Dump mechanism for unparseable descriptors */ -static void dump_desc_populate_fifo_from_directory(const char *dirname); - /** List of dumped descriptors for FIFO cleanup purposes */ STATIC smartlist_t *descs_dumped = NULL; /** Total size of dumped descriptors for FIFO cleanup */ @@ -972,7 +970,7 @@ dump_desc_compare_fifo_entries(const void **a_v, const void **b_v) * reconstructed order will be wrong, but will always be a permutation of * the original. */ -static void +STATIC void dump_desc_populate_fifo_from_directory(const char *dirname) { smartlist_t *files = NULL; diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 131f158c56..2eb9932410 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -109,6 +109,7 @@ STATIC int routerstatus_parse_guardfraction(const char *guardfraction_str, 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 diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 2c398e36c3..873426c32b 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -4865,6 +4865,7 @@ read_file_to_str_mock(const char *filename, int flags, return result; } +/* This one tests dump_desc_populate_one_file() */ static void test_dir_populate_dump_desc_fifo(void *data) { @@ -4996,6 +4997,97 @@ test_dir_populate_dump_desc_fifo(void *data) 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) @@ -5207,6 +5299,7 @@ struct testcase_t dir_tests[] = { 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"), -- cgit v1.2.3-54-g00ecf From 34d9dabed18452aed6ba0d983673c8366c1b64a2 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 07:45:55 +0000 Subject: Do sandbox_is_active() check before reconstructing dump_desc() FIFO on startup too --- src/or/routerparse.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 2260693939..def6373357 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -6001,7 +6001,9 @@ rend_parse_client_keys(strmap_t *parsed_clients, const char *ckstr) void routerparse_init(void) { - dump_desc_init(); + if (!(sandbox_is_active())) { + dump_desc_init(); + } } /** Clean up all data structures used by routerparse.c at exit */ -- cgit v1.2.3-54-g00ecf From 13a16e001164581b687dae2d3377f77eedb701ff Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 09:37:23 +0000 Subject: Also check if the sandbox is configured as well as if it's active; sandbox_init() runs rather late in the startup process --- src/or/routerparse.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/or/routerparse.c b/src/or/routerparse.c index def6373357..875bb605b0 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1073,7 +1073,11 @@ dump_desc(const char *desc, const char *type) DESC_DUMP_BASE_FILENAME ".%s", digest_sha256_hex); debugfile = get_datadir_fname2(DESC_DUMP_DATADIR_SUBDIR, debugfile_base); - if (!sandbox_is_active()) { + /* + * 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 */ @@ -1128,7 +1132,7 @@ dump_desc(const char *desc, const char *type) log_info(LD_DIR, "Unable to parse descriptor of type %s with hash %s and " "length %lu. Descriptor not dumped because the sandbox is " - "active", + "configured", type, digest_sha256_hex, (unsigned long)len); } @@ -6001,7 +6005,12 @@ rend_parse_client_keys(strmap_t *parsed_clients, const char *ckstr) void routerparse_init(void) { - if (!(sandbox_is_active())) { + /* + * 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(); } } -- cgit v1.2.3-54-g00ecf