commit 38cced90ef62aff04477d1b814ab2ee3b7776638 Author: Andrea Shepard andrea@torproject.org Date: Thu Jun 30 00:39:29 2016 +0000

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(-)

diff --git a/src/common/util.c b/src/common/util.c index d4553c3..d60380c 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 fd9b983..70483bb 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 3291570..65a67a9 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 93b90cc..75c09d2 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 2b18143..a96146a 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 e18ed42..b4fc615 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -4103,6 +4103,22 @@ test_dir_choose_compression_level(void* data) }

/* + * Mock check_private_dir(), and always succeed - no need to actually + * look at or create anything on the filesystem. + */ + +static int +mock_check_private_dir(const char *dirname, cpd_check_t check, + const char *effective_user) +{ + (void)dirname; + (void)check; + (void)effective_user; + + return 0; +} + +/* * This really mocks options_get_datadir_fname2_suffix(), but for testing * dump_desc(), we only care about get_datadir_fname(sub1), which is defined * in config.h as: @@ -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;