From 5b7eaa3765b8c616c93dbda26a02c780d5c95084 Mon Sep 17 00:00:00 2001 From: Peter Retzlaff Date: Fri, 25 Jan 2013 11:49:33 +0100 Subject: [PATCH 1/6] Extract duplicate code in geoip and rephist. Create new methods check_or_create_data_subdir() and write_to_data_subdir() in config.c and use them throughout rephist.c and geoip.c. This should solve ticket #4282. --- src/or/config.c | 37 +++++++++++++++++++++++++++ src/or/config.h | 4 +++ src/or/geoip.c | 65 +++++++++++++++++------------------------------- src/or/rephist.c | 48 +++++++++-------------------------- 4 files changed, 76 insertions(+), 78 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 8ca89b6a77..554ccb60cd 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -5961,6 +5961,43 @@ options_get_datadir_fname2_suffix(const or_options_t *options, return fname; } +/** Check wether the data directory has a private subdirectory + * subdir. If not, try to create it. Return 0 on success, + * -1 otherwise. */ +int +check_or_create_data_subdir(const char *subdir) +{ + char *statsdir = get_datadir_fname(subdir); + int return_val = 0; + + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { + log_warn(LD_HIST, "Unable to create %s/ directory!", subdir); + return_val = -1; + } + tor_free(statsdir); + return return_val; +} + +/** Create a file named fname with contents str in the + * subdirectory subdir of the data directory. descr + * should be a short description of the file's content and will be + * used for the warning message, if it's present and the write process + * fails. Return 0 on success, -1 otherwise.*/ +int +write_to_data_subdir(const char* subdir, const char* fname, + const char* str, const char* descr) +{ + char *filename = get_datadir_fname2(subdir, fname); + int return_val = 0; + + if (write_str_to_file(filename, str, 0) < 0) { + log_warn(LD_HIST, "Unable to write %s to disk!", descr ? descr : fname); + return_val = -1; + } + tor_free(filename); + return return_val; +} + /** Given a file name check to see whether the file exists but has not been * modified for a very long time. If so, remove it. */ void diff --git a/src/or/config.h b/src/or/config.h index fbdedcfb50..0250f645d0 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -59,6 +59,10 @@ char *options_get_datadir_fname2_suffix(const or_options_t *options, #define get_datadir_fname_suffix(sub1, suffix) \ get_datadir_fname2_suffix((sub1), NULL, (suffix)) +int check_or_create_data_subdir(const char *subdir); +int write_to_data_subdir(const char* subdir, const char* fname, + const char* str, const char* descr); + int get_num_cpus(const or_options_t *options); const smartlist_t *get_configured_ports(void); diff --git a/src/or/geoip.c b/src/or/geoip.c index e2e98e8ec4..73ad211881 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -1132,7 +1132,7 @@ geoip_format_dirreq_stats(time_t now) time_t geoip_dirreq_stats_write(time_t now) { - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *str = NULL; if (!start_of_dirreq_stats_interval) return 0; /* Not initialized. */ @@ -1146,21 +1146,13 @@ geoip_dirreq_stats_write(time_t now) str = geoip_format_dirreq_stats(now); /* Write dirreq-stats string to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; + if (check_or_create_data_subdir("stats") < 0) { + write_to_data_subdir("stats", "dirreq-stats", str, "dirreq statistics"); + /* Reset measurement interval start. */ + geoip_reset_dirreq_stats(now); } - filename = get_datadir_fname2("stats", "dirreq-stats"); - if (write_str_to_file(filename, str, 0) < 0) - log_warn(LD_HIST, "Unable to write dirreq statistics to disk!"); - - /* Reset measurement interval start. */ - geoip_reset_dirreq_stats(now); done: - tor_free(statsdir); - tor_free(filename); tor_free(str); return start_of_dirreq_stats_interval + WRITE_STATS_INTERVAL; } @@ -1297,7 +1289,7 @@ format_bridge_stats_controller(time_t now) time_t geoip_bridge_stats_write(time_t now) { - char *filename = NULL, *val = NULL, *statsdir = NULL; + char *val = NULL; /* Check if 24 hours have passed since starting measurements. */ if (now < start_of_bridge_stats_interval + WRITE_STATS_INTERVAL) @@ -1317,24 +1309,20 @@ geoip_bridge_stats_write(time_t now) start_of_bridge_stats_interval = now; /* Write it to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) - goto done; - filename = get_datadir_fname2("stats", "bridge-stats"); + if (!check_or_create_data_subdir("stats")) { + write_to_data_subdir("stats", "bridge-stats", + bridge_stats_extrainfo, "bridge statistics"); - write_str_to_file(filename, bridge_stats_extrainfo, 0); - - /* Tell the controller, "hey, there are clients!" */ - { - char *controller_str = format_bridge_stats_controller(now); - if (controller_str) - control_event_clients_seen(controller_str); - tor_free(controller_str); + /* Tell the controller, "hey, there are clients!" */ + { + char *controller_str = format_bridge_stats_controller(now); + if (controller_str) + control_event_clients_seen(controller_str); + tor_free(controller_str); + } } - done: - tor_free(filename); - tor_free(statsdir); + done: return start_of_bridge_stats_interval + WRITE_STATS_INTERVAL; } @@ -1436,7 +1424,7 @@ geoip_format_entry_stats(time_t now) time_t geoip_entry_stats_write(time_t now) { - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *str = NULL; if (!start_of_entry_stats_interval) return 0; /* Not initialized. */ @@ -1450,21 +1438,14 @@ geoip_entry_stats_write(time_t now) str = geoip_format_entry_stats(now); /* Write entry-stats string to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; - } - filename = get_datadir_fname2("stats", "entry-stats"); - if (write_str_to_file(filename, str, 0) < 0) - log_warn(LD_HIST, "Unable to write entry statistics to disk!"); + if (!check_or_create_data_subdir("stats")) { + write_to_data_subdir("stats", "entry-stats", str, "entry statistics"); - /* Reset measurement interval start. */ - geoip_reset_entry_stats(now); + /* Reset measurement interval start. */ + geoip_reset_entry_stats(now); + } done: - tor_free(statsdir); - tor_free(filename); tor_free(str); return start_of_entry_stats_interval + WRITE_STATS_INTERVAL; } diff --git a/src/or/rephist.c b/src/or/rephist.c index 55f321d5ff..c84322a679 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2313,7 +2313,7 @@ rep_hist_format_exit_stats(time_t now) time_t rep_hist_exit_stats_write(time_t now) { - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *str = NULL; if (!start_of_exit_stats_interval) return 0; /* Not initialized. */ @@ -2329,19 +2329,12 @@ rep_hist_exit_stats_write(time_t now) rep_hist_reset_exit_stats(now); /* Try to write to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; + if (!check_or_create_data_subdir("stats")) { + write_to_data_subdir("stats", "exit-stats", str, "exit port statistics"); } - filename = get_datadir_fname2("stats", "exit-stats"); - if (write_str_to_file(filename, str, 0) < 0) - log_warn(LD_HIST, "Unable to write exit port statistics to disk!"); done: tor_free(str); - tor_free(statsdir); - tor_free(filename); return start_of_exit_stats_interval + WRITE_STATS_INTERVAL; } @@ -2598,7 +2591,7 @@ time_t rep_hist_buffer_stats_write(time_t now) { circuit_t *circ; - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *str = NULL; if (!start_of_buffer_stats_interval) return 0; /* Not initialized. */ @@ -2617,19 +2610,12 @@ rep_hist_buffer_stats_write(time_t now) rep_hist_reset_buffer_stats(now); /* Try to write to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; + if (!check_or_create_data_subdir("stats")) { + write_to_data_subdir("stats", "buffer-stats", str, "buffer statistics"); } - filename = get_datadir_fname2("stats", "buffer-stats"); - if (write_str_to_file(filename, str, 0) < 0) - log_warn(LD_HIST, "Unable to write buffer stats to disk!"); done: tor_free(str); - tor_free(filename); - tor_free(statsdir); return start_of_buffer_stats_interval + WRITE_STATS_INTERVAL; } @@ -2741,7 +2727,7 @@ rep_hist_format_desc_stats(time_t now) time_t rep_hist_desc_stats_write(time_t now) { - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *filename = NULL, *str = NULL; if (!start_of_served_descs_stats_interval) return 0; /* We're not collecting stats. */ @@ -2751,10 +2737,8 @@ rep_hist_desc_stats_write(time_t now) str = rep_hist_format_desc_stats(now); tor_assert(str != NULL); - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; + if (check_or_create_data_subdir("stats") < 0) { + goto done; } filename = get_datadir_fname2("stats", "served-desc-stats"); if (append_bytes_to_file(filename, str, strlen(str), 0) < 0) @@ -2763,7 +2747,6 @@ rep_hist_desc_stats_write(time_t now) rep_hist_reset_desc_stats(now); done: - tor_free(statsdir); tor_free(filename); tor_free(str); return start_of_served_descs_stats_interval + WRITE_STATS_INTERVAL; @@ -2981,7 +2964,7 @@ rep_hist_format_conn_stats(time_t now) time_t rep_hist_conn_stats_write(time_t now) { - char *statsdir = NULL, *filename = NULL, *str = NULL; + char *str = NULL; if (!start_of_conn_stats_interval) return 0; /* Not initialized. */ @@ -2995,19 +2978,12 @@ rep_hist_conn_stats_write(time_t now) rep_hist_reset_conn_stats(now); /* Try to write to disk. */ - statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { - log_warn(LD_HIST, "Unable to create stats/ directory!"); - goto done; + if (!check_or_create_data_subdir("stats")) { + write_to_data_subdir("stats", "conn-stats", str, "connection statistics"); } - filename = get_datadir_fname2("stats", "conn-stats"); - if (write_str_to_file(filename, str, 0) < 0) - log_warn(LD_HIST, "Unable to write conn stats to disk!"); done: tor_free(str); - tor_free(filename); - tor_free(statsdir); return start_of_conn_stats_interval + WRITE_STATS_INTERVAL; } From 78cc5833a1da038331186ddf07f4add7f8f1094b Mon Sep 17 00:00:00 2001 From: Peter Retzlaff Date: Sat, 2 Feb 2013 01:40:41 +0100 Subject: [PATCH 2/6] Unit tests for check_or_create_data_subdir and write_to_data_subdir. --- src/test/test_config.c | 115 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/src/test/test_config.c b/src/test/test_config.c index d1e7ccf597..78def700ca 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -170,6 +170,119 @@ test_config_addressmap(void *arg) done: ; } + +static int +is_private_dir(const char* path) +{ + struct stat st; + int r = stat(path, &st); + if (r) { + return 0; + } +#if !defined (_WIN32) || defined (WINCE) + if (st.st_mode != (S_IFDIR | 0700)) { + return 0; + } +#endif + return 1; +} + +static void +test_config_check_or_create_data_subdir(void *arg) +{ + or_options_t* options = get_options_mutable(); + options->DataDirectory = "test_data"; + const char* subdir = "test_stats"; + const char* subpath = get_datadir_fname(subdir); + struct stat st; + +#if defined (_WIN32) && !defined (WINCE) + mkdir(options->DataDirectory); +#else + mkdir(options->DataDirectory, 0700); +#endif + + int r = stat(subpath, &st); + + // The subdirectory shouldn't exist yet, + // but should be created by the call to check_or_create_data_subdir. + test_assert(r && (errno == ENOENT)); + test_assert(!check_or_create_data_subdir(subdir)); + test_assert(is_private_dir(subpath)); + + // The check should return 0, if the directory already exists + // and is private to the user. + test_assert(!check_or_create_data_subdir(subdir)); + +#if !defined (_WIN32) || defined (WINCE) + unsigned group_permission = st.st_mode | 0070; + r = chmod(subpath, group_permission); + + if (r) { + test_fail_msg("Changing permissions for the subdirectory failed."); + } + + // If the directory exists, but its mode is too permissive + // a call to check_or_create_data_subdir should reset the mode. + test_assert(!is_private_dir(subpath)); + test_assert(!check_or_create_data_subdir(subdir)); + test_assert(is_private_dir(subpath)); +#endif + + done: + rmdir(subpath); + rmdir(options->DataDirectory); +} + +static void +test_config_write_to_data_subdir(void* arg) +{ + or_options_t* options = get_options_mutable(); + options->DataDirectory = "test_data"; + const char* subdir = "test_stats"; + const char* fname = "test_file"; + const char* str = + "Lorem ipsum dolor sit amet, consetetur sadipscing\n" + "elitr, sed diam nonumy eirmod\n" + "tempor invidunt ut labore et dolore magna aliquyam\n" + "erat, sed diam voluptua.\n" + "At vero eos et accusam et justo duo dolores et ea\n" + "rebum. Stet clita kasd gubergren,\n" + "no sea takimata sanctus est Lorem ipsum dolor sit amet.\n" + "Lorem ipsum dolor sit amet,\n" + "consetetur sadipscing elitr, sed diam nonumy eirmod\n" + "tempor invidunt ut labore et dolore\n" + "magna aliquyam erat, sed diam voluptua. At vero eos et\n" + "accusam et justo duo dolores et\n" + "ea rebum. Stet clita kasd gubergren, no sea takimata\n" + "sanctus est Lorem ipsum dolor sit amet."; + const char* subpath = get_datadir_fname(subdir); + const char* filepath = get_datadir_fname2(subdir, fname); + +#if defined (_WIN32) && !defined (WINCE) + mkdir(options->DataDirectory); +#else + mkdir(options->DataDirectory, 0700); +#endif + + // Write attempt shoudl fail, if subdirectory doesn't exist. + test_assert(write_to_data_subdir(subdir, fname, str, NULL)); + check_or_create_data_subdir(subdir); + + // Content of file after write attempt should be + // equal to the original string. + test_assert(!write_to_data_subdir(subdir, fname, str, NULL)); + test_streq(read_file_to_str(filepath, 0, NULL), str); + + // A second write operation should overwrite the old content. + test_assert(!write_to_data_subdir(subdir, fname, str, NULL)); + test_streq(read_file_to_str(filepath, 0, NULL), str); + + done: + remove(filepath); + rmdir(subpath); + rmdir(options->DataDirectory); +} /* Test helper function: Make sure that a bridge line gets parsed * properly. Also make sure that the resulting bridge_line_t structure @@ -324,6 +437,8 @@ test_config_parse_bridge_line(void *arg) struct testcase_t config_tests[] = { CONFIG_TEST(addressmap, 0), CONFIG_TEST(parse_bridge_line, 0), + CONFIG_TEST(check_or_create_data_subdir, 0), + CONFIG_TEST(write_to_data_subdir, 0), END_OF_TESTCASES }; From 58721ac24c684236c52c062a255c38f2f019fda5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 24 May 2013 13:31:10 -0400 Subject: [PATCH 3/6] Tweak 4282 unit tests for style, safety, correctness. We do our filesystem wrangling relative to get_fname() results, so that if we fail or crash, we can always clean up. --- src/test/test_config.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/test/test_config.c b/src/test/test_config.c index 78def700ca..8316d0eda4 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -170,7 +170,7 @@ test_config_addressmap(void *arg) done: ; } - + static int is_private_dir(const char* path) { @@ -180,7 +180,7 @@ is_private_dir(const char* path) return 0; } #if !defined (_WIN32) || defined (WINCE) - if (st.st_mode != (S_IFDIR | 0700)) { + if ((st.st_mode & (S_IFDIR | 0777)) != (S_IFDIR | 0700)) { return 0; } #endif @@ -190,11 +190,14 @@ is_private_dir(const char* path) static void test_config_check_or_create_data_subdir(void *arg) { - or_options_t* options = get_options_mutable(); - options->DataDirectory = "test_data"; - const char* subdir = "test_stats"; - const char* subpath = get_datadir_fname(subdir); + or_options_t *options = get_options_mutable(); + char *datadir = options->DataDirectory = tor_strdup(get_fname("datadir-0")); + const char *subdir = "test_stats"; + const char *subpath = get_datadir_fname(subdir); struct stat st; + int r; + unsigned group_permission; + (void)arg; #if defined (_WIN32) && !defined (WINCE) mkdir(options->DataDirectory); @@ -202,7 +205,7 @@ test_config_check_or_create_data_subdir(void *arg) mkdir(options->DataDirectory, 0700); #endif - int r = stat(subpath, &st); + r = stat(subpath, &st); // The subdirectory shouldn't exist yet, // but should be created by the call to check_or_create_data_subdir. @@ -215,7 +218,7 @@ test_config_check_or_create_data_subdir(void *arg) test_assert(!check_or_create_data_subdir(subdir)); #if !defined (_WIN32) || defined (WINCE) - unsigned group_permission = st.st_mode | 0070; + group_permission = st.st_mode | 0070; r = chmod(subpath, group_permission); if (r) { @@ -229,16 +232,16 @@ test_config_check_or_create_data_subdir(void *arg) test_assert(is_private_dir(subpath)); #endif - done: - rmdir(subpath); - rmdir(options->DataDirectory); + done: + rmdir(subpath); + tor_free(datadir); } static void -test_config_write_to_data_subdir(void* arg) +test_config_write_to_data_subdir(void *arg) { or_options_t* options = get_options_mutable(); - options->DataDirectory = "test_data"; + char *datadir = options->DataDirectory = tor_strdup(get_fname("datadir-1")); const char* subdir = "test_stats"; const char* fname = "test_file"; const char* str = @@ -258,6 +261,7 @@ test_config_write_to_data_subdir(void* arg) "sanctus est Lorem ipsum dolor sit amet."; const char* subpath = get_datadir_fname(subdir); const char* filepath = get_datadir_fname2(subdir, fname); + (void)arg; #if defined (_WIN32) && !defined (WINCE) mkdir(options->DataDirectory); @@ -282,6 +286,7 @@ test_config_write_to_data_subdir(void* arg) remove(filepath); rmdir(subpath); rmdir(options->DataDirectory); + tor_free(datadir); } /* Test helper function: Make sure that a bridge line gets parsed @@ -437,8 +442,8 @@ test_config_parse_bridge_line(void *arg) struct testcase_t config_tests[] = { CONFIG_TEST(addressmap, 0), CONFIG_TEST(parse_bridge_line, 0), - CONFIG_TEST(check_or_create_data_subdir, 0), - CONFIG_TEST(write_to_data_subdir, 0), + CONFIG_TEST(check_or_create_data_subdir, TT_FORK), + CONFIG_TEST(write_to_data_subdir, TT_FORK), END_OF_TESTCASES }; From 57e4324c425e6c9acd58c2270a183ee4d9b1e4aa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 24 May 2013 13:36:15 -0400 Subject: [PATCH 4/6] Fix a logic error in 4282 fixes check_or_create_data_subdir has succeeded when it returns 0, not when it returns negative. --- src/or/geoip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/geoip.c b/src/or/geoip.c index 73ad211881..1c307dcc57 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -1146,7 +1146,7 @@ geoip_dirreq_stats_write(time_t now) str = geoip_format_dirreq_stats(now); /* Write dirreq-stats string to disk. */ - if (check_or_create_data_subdir("stats") < 0) { + if (!check_or_create_data_subdir("stats")) { write_to_data_subdir("stats", "dirreq-stats", str, "dirreq statistics"); /* Reset measurement interval start. */ geoip_reset_dirreq_stats(now); From eef42d3863f8dd5003c011833d8327d17623c675 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 24 May 2013 13:37:14 -0400 Subject: [PATCH 5/6] Reformat 4282 fixes a little --- src/or/geoip.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/geoip.c b/src/or/geoip.c index 1c307dcc57..d6e8ee0d06 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -1311,7 +1311,7 @@ geoip_bridge_stats_write(time_t now) /* Write it to disk. */ if (!check_or_create_data_subdir("stats")) { write_to_data_subdir("stats", "bridge-stats", - bridge_stats_extrainfo, "bridge statistics"); + bridge_stats_extrainfo, "bridge statistics"); /* Tell the controller, "hey, there are clients!" */ { @@ -1439,10 +1439,10 @@ geoip_entry_stats_write(time_t now) /* Write entry-stats string to disk. */ if (!check_or_create_data_subdir("stats")) { - write_to_data_subdir("stats", "entry-stats", str, "entry statistics"); + write_to_data_subdir("stats", "entry-stats", str, "entry statistics"); - /* Reset measurement interval start. */ - geoip_reset_entry_stats(now); + /* Reset measurement interval start. */ + geoip_reset_entry_stats(now); } done: From c482c7122b3be1407edc8dd2e85dc4e9390002a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 24 May 2013 13:39:21 -0400 Subject: [PATCH 6/6] changes file for 4282 --- changes/bug4282 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug4282 diff --git a/changes/bug4282 b/changes/bug4282 new file mode 100644 index 0000000000..4d4f4896fe --- /dev/null +++ b/changes/bug4282 @@ -0,0 +1,4 @@ + o Code simplifications and refactoring: + - Extract the common duplicated code for creating a subdirectory + of the data directory and writing to a file in it. Fixes ticket + 4282; patch from Peter Retzlaff.