From db7dd3ee7a07d9bc121d1c8f353e206327bb72c5 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 20:02:16 +0200 Subject: [PATCH 1/7] remove some dead code, found by coverity --- changes/coverity_maint | 3 +++ src/or/rephist.c | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 changes/coverity_maint diff --git a/changes/coverity_maint b/changes/coverity_maint new file mode 100644 index 0000000000..ec25d097e1 --- /dev/null +++ b/changes/coverity_maint @@ -0,0 +1,3 @@ + o Code simplifications and refactoring: + - Remove some dead code as indicated by coverity. + diff --git a/src/or/rephist.c b/src/or/rephist.c index 6be8484cc5..242fe81d52 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2401,8 +2401,7 @@ rep_hist_buffer_stats_add_circ(circuit_t *circ, time_t end_of_interval) stat = tor_malloc_zero(sizeof(circ_buffer_stats_t)); stat->processed_cells = orcirc->processed_cells; /* 1000.0 for s -> ms; 2.0 because of app-ward and exit-ward queues */ - stat->mean_num_cells_in_queue = interval_length == 0 ? 0.0 : - (double) orcirc->total_cell_waiting_time / + stat->mean_num_cells_in_queue = (double) orcirc->total_cell_waiting_time / (double) interval_length / 1000.0 / 2.0; stat->mean_time_cells_in_queue = (double) orcirc->total_cell_waiting_time / From ff75e8b02dbf35c2ecc82ffd4d2b2112a144322e Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 21:06:01 +0200 Subject: [PATCH 2/7] Check some more return values in unit tests --- changes/coverity_maint | 3 +++ src/test/test_addr.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/changes/coverity_maint b/changes/coverity_maint index ec25d097e1..1bd1c417fb 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -1,3 +1,6 @@ o Code simplifications and refactoring: - Remove some dead code as indicated by coverity. + o Minor bugfixes: + - Add some forgotten return value checks during unit tests. Found + by coverity. diff --git a/src/test/test_addr.c b/src/test/test_addr.c index 6db4ee2483..1dab0e0112 100644 --- a/src/test/test_addr.c +++ b/src/test/test_addr.c @@ -436,13 +436,16 @@ test_addr_ip6_helpers(void) /* test tor_addr_parse_mask_ports */ test_addr_mask_ports_parse("[::f]/17:47-95", AF_INET6, 0, 0, 0, 0x0000000f, 17, 47, 95); + test_streq(p1, "::f"); //test_addr_parse("[::fefe:4.1.1.7/120]:999-1000"); //test_addr_parse_check("::fefe:401:107", 120, 999, 1000); test_addr_mask_ports_parse("[::ffff:4.1.1.7]/120:443", AF_INET6, 0, 0, 0x0000ffff, 0x04010107, 120, 443, 443); + test_streq(p1, "::ffff:4.1.1.7"); test_addr_mask_ports_parse("[abcd:2::44a:0]:2-65000", AF_INET6, 0xabcd0002, 0, 0, 0x044a0000, 128, 2, 65000); + test_streq(p1, "abcd:2::44a:0"); r=tor_addr_parse_mask_ports("[fefef::]/112", &t1, NULL, NULL, NULL); test_assert(r == -1); r=tor_addr_parse_mask_ports("efef::/112", &t1, NULL, NULL, NULL); From e6fff7235e46b794cd77af516a1ecfab9c87de7f Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 21:16:11 +0200 Subject: [PATCH 3/7] Remove a few dead assignments during router parsing --- changes/coverity_maint | 1 + src/or/routerparse.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index 1bd1c417fb..fd8c440505 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -1,5 +1,6 @@ o Code simplifications and refactoring: - Remove some dead code as indicated by coverity. + - Remove a few dead assignments during router parsing. Found by coverity. o Minor bugfixes: - Add some forgotten return value checks during unit tests. Found by coverity. diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f855f9d027..42dbcacb51 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1544,10 +1544,10 @@ router_parse_entry_from_string(const char *s, const char *end, } } - if ((tok = find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO))) + if (find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO)) router->caches_extra_info = 1; - if ((tok = find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS))) + if (find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS)) router->allow_single_hop_exits = 1; if ((tok = find_opt_by_keyword(tokens, K_EXTRA_INFO_DIGEST))) { @@ -1560,7 +1560,7 @@ router_parse_entry_from_string(const char *s, const char *end, } } - if ((tok = find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR))) { + if (find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR)) { router->wants_to_be_hs_dir = 1; } From 9e56ac27da80969fadaad7eaf64a8384e68a778e Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 21:23:05 +0200 Subject: [PATCH 4/7] Comment out some obviously dead code. Coverity warned about it, it's harmless to comment out. --- src/or/circuitlist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 4ad244dfda..8534c38075 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -775,8 +775,8 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn) return found->circuit; return NULL; - /* The rest of this checks for bugs. Disabled by default. */ + /* We comment it out because coverity complains otherwise. { circuit_t *circ; for (circ=global_circuitlist;circ;circ = circ->next) { @@ -795,7 +795,7 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn) } } return NULL; - } + } */ } /** Return a circ such that: From 680646e0de29454f92d57bc3a4895d75c95e158c Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 21:27:32 +0200 Subject: [PATCH 5/7] Don't use signed 1-bit bitfields This was harmless, we never compared it to anything but itself or 0. But Coverity complained, and it had a point. --- changes/coverity_maint | 1 + src/or/policies.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index fd8c440505..6d60355b11 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -4,4 +4,5 @@ o Minor bugfixes: - Add some forgotten return value checks during unit tests. Found by coverity. + - Don't use 1-bit wide signed bit fields. Found by coverity. diff --git a/src/or/policies.c b/src/or/policies.c index e48f42058f..c87036013d 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -45,7 +45,7 @@ typedef struct policy_summary_item_t { uint16_t prt_max; /**< Highest port number to accept/reject. */ uint64_t reject_count; /**< Number of IP-Addresses that are rejected to this port range. */ - int accepted:1; /** Has this port already been accepted */ + unsigned int accepted:1; /** Has this port already been accepted */ } policy_summary_item_t; /** Private networks. This list is used in two places, once to expand the From f30327449009a7f00b0f5c2bd09a7eff615df3dd Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Wed, 8 Jun 2011 21:35:26 +0200 Subject: [PATCH 6/7] Fix a rare memleak during stats writing If rep_hist_buffer_stats_write() was called unitinitalized, we'd leak memory. --- changes/coverity_maint | 1 + src/or/rephist.c | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index 6d60355b11..e7be90a485 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -5,4 +5,5 @@ - Add some forgotten return value checks during unit tests. Found by coverity. - Don't use 1-bit wide signed bit fields. Found by coverity. + - Fix a rare memory leak during stats writing. Found by coverity. diff --git a/src/or/rephist.c b/src/or/rephist.c index 242fe81d52..54593a06c3 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2451,8 +2451,8 @@ rep_hist_buffer_stats_write(time_t now) int processed_cells[SHARES], circs_in_share[SHARES], number_of_circuits, i; double queued_cells[SHARES], time_in_queue[SHARES]; - smartlist_t *str_build = smartlist_create(); - char *str = NULL, *buf=NULL; + smartlist_t *str_build = NULL; + char *str = NULL, *buf = NULL; circuit_t *circ; if (!start_of_buffer_stats_interval) @@ -2460,6 +2460,8 @@ rep_hist_buffer_stats_write(time_t now) if (start_of_buffer_stats_interval + WRITE_STATS_INTERVAL > now) goto done; /* Not ready to write */ + str_build = smartlist_create(); + /* add current circuits to stats */ for (circ = _circuit_get_global_list(); circ; circ = circ->next) rep_hist_buffer_stats_add_circ(circ, now); From 54d7d31cba84232b50fef4287951b2c4bfa746c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Bobbio?= Date: Tue, 14 Jun 2011 12:18:32 -0400 Subject: [PATCH 7/7] Make ControlSocketsGroupWritable work with User. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original message from bug3393: check_private_dir() to ensure that ControlSocketsGroupWritable is safe to use. Unfortunately, check_private_dir() only checks against the currently running user… which can be root until privileges are dropped to the user and group configured by the User config option. The attached patch fixes the issue by adding a new effective_user argument to check_private_dir() and updating the callers. It might not be the best way to fix the issue, but it did in my tests. (Code by lunar; changelog by nickm) --- src/common/util.c | 33 ++++++++++++++++++++++++++------- src/common/util.h | 3 ++- src/or/config.c | 6 ++++-- src/or/connection.c | 2 +- src/or/geoip.c | 6 +++--- src/or/rendservice.c | 2 +- src/or/rephist.c | 4 ++-- src/or/router.c | 4 ++-- 8 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 6f323dd20c..36aa9ccef8 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -1677,15 +1677,20 @@ file_status(const char *fname) * is group-readable, but in all cases we create the directory mode 0700. * If CPD_CHECK_MODE_ONLY is set, then we don't alter the directory permissions * if they are too permissive: we just return -1. + * 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) +check_private_dir(const char *dirname, cpd_check_t check, const char *effective_user) { int r; struct stat st; char *f; #ifndef MS_WINDOWS int mask; + struct passwd *pw = NULL; + uid_t running_uid; + gid_t running_gid; #endif tor_assert(dirname); @@ -1724,33 +1729,47 @@ check_private_dir(const char *dirname, cpd_check_t check) return -1; } #ifndef MS_WINDOWS - if (st.st_uid != getuid()) { + if (effective_user) { + /* Lookup the user and group information, if we have a problem, bail out. */ + pw = getpwnam(effective_user); + if (pw == NULL) { + log_warn(LD_CONFIG, "Error setting configured user: %s not found", effective_user); + return -1; + } + running_uid = pw->pw_uid; + running_gid = pw->pw_gid; + } else { + running_uid = getuid(); + running_gid = getgid(); + } + + if (st.st_uid != running_uid) { struct passwd *pw = NULL; char *process_ownername = NULL; - pw = getpwuid(getuid()); + pw = getpwuid(running_uid); process_ownername = pw ? tor_strdup(pw->pw_name) : tor_strdup(""); pw = getpwuid(st.st_uid); log_warn(LD_FS, "%s is not owned by this user (%s, %d) but by " "%s (%d). Perhaps you are running Tor as the wrong user?", - dirname, process_ownername, (int)getuid(), + dirname, process_ownername, (int)running_uid, pw ? pw->pw_name : "", (int)st.st_uid); tor_free(process_ownername); return -1; } - if ((check & CPD_GROUP_OK) && st.st_gid != getgid()) { + if ((check & CPD_GROUP_OK) && st.st_gid != running_gid) { struct group *gr; char *process_groupname = NULL; - gr = getgrgid(getgid()); + gr = getgrgid(running_gid); process_groupname = gr ? tor_strdup(gr->gr_name) : tor_strdup(""); gr = getgrgid(st.st_gid); log_warn(LD_FS, "%s is not owned by this group (%s, %d) but by group " "%s (%d). Are you running Tor as the wrong user?", - dirname, process_groupname, (int)getgid(), + dirname, process_groupname, (int)running_gid, gr ? gr->gr_name : "", (int)st.st_gid); tor_free(process_groupname); diff --git a/src/common/util.h b/src/common/util.h index d657db674e..b9db25ca73 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -292,7 +292,8 @@ typedef unsigned int cpd_check_t; #define CPD_CHECK 2 #define CPD_GROUP_OK 4 #define CPD_CHECK_MODE_ONLY 8 -int check_private_dir(const char *dirname, cpd_check_t check); +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) typedef struct open_file_t open_file_t; diff --git a/src/or/config.c b/src/or/config.c index 44cecf353b..8ab23a3b80 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1025,7 +1025,8 @@ options_act_reversible(or_options_t *old_options, char **msg) /* Ensure data directory is private; create if possible. */ if (check_private_dir(options->DataDirectory, - running_tor ? CPD_CREATE : CPD_CHECK)<0) { + running_tor ? CPD_CREATE : CPD_CHECK, + options->User)<0) { tor_asprintf(msg, "Couldn't access/create private data directory \"%s\"", options->DataDirectory); @@ -1038,7 +1039,8 @@ options_act_reversible(or_options_t *old_options, char **msg) char *fn = tor_malloc(len); tor_snprintf(fn, len, "%s"PATH_SEPARATOR"cached-status", options->DataDirectory); - if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK) < 0) { + if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK, + options->User) < 0) { tor_asprintf(msg, "Couldn't access/create private data directory \"%s\"", fn); tor_free(fn); diff --git a/src/or/connection.c b/src/or/connection.c index 3f4ca1db4b..a9e3a74ed6 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -867,7 +867,7 @@ check_location_for_unix_socket(or_options_t *options, const char *path) if (options->ControlSocketsGroupWritable) flags |= CPD_GROUP_OK; - if (check_private_dir(p, flags) < 0) { + if (check_private_dir(p, flags, options->User) < 0) { char *escpath, *escdir; escpath = esc_for_log(path); escdir = esc_for_log(p); diff --git a/src/or/geoip.c b/src/or/geoip.c index 5bb2410a75..c621ea8183 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -970,7 +970,7 @@ geoip_dirreq_stats_write(time_t now) geoip_remove_old_clients(start_of_dirreq_stats_interval); statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) goto done; filename = get_datadir_fname2("stats", "dirreq-stats"); data_v2 = geoip_get_client_history(GEOIP_CLIENT_NETWORKSTATUS_V2); @@ -1209,7 +1209,7 @@ geoip_bridge_stats_write(time_t now) /* Write it to disk. */ statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) goto done; filename = get_datadir_fname2("stats", "bridge-stats"); @@ -1304,7 +1304,7 @@ geoip_entry_stats_write(time_t now) geoip_remove_old_clients(start_of_entry_stats_interval); statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) goto done; filename = get_datadir_fname2("stats", "entry-stats"); data = geoip_get_client_history(GEOIP_CLIENT_CONNECT); diff --git a/src/or/rendservice.c b/src/or/rendservice.c index a10e43fead..d9a936471e 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -569,7 +569,7 @@ rend_service_load_keys(void) s->directory); /* Check/create directory */ - if (check_private_dir(s->directory, CPD_CREATE) < 0) + if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0) return -1; /* Load key */ diff --git a/src/or/rephist.c b/src/or/rephist.c index 54593a06c3..b7341f3c0d 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2307,7 +2307,7 @@ rep_hist_exit_stats_write(time_t now) /* Try to write to disk. */ statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) { + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { log_warn(LD_HIST, "Unable to create stats/ directory!"); goto done; } @@ -2497,7 +2497,7 @@ rep_hist_buffer_stats_write(time_t now) smartlist_clear(circuits_for_buffer_stats); /* write to file */ statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) goto done; filename = get_datadir_fname2("stats", "buffer-stats"); out = start_writing_to_stdio_file(filename, OPEN_FLAGS_APPEND, diff --git a/src/or/router.c b/src/or/router.c index 68e29bb4c8..2165e6ea90 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -533,12 +533,12 @@ init_keys(void) return 0; } /* Make sure DataDirectory exists, and is private. */ - if (check_private_dir(options->DataDirectory, CPD_CREATE)) { + if (check_private_dir(options->DataDirectory, CPD_CREATE, options->User)) { return -1; } /* Check the key directory. */ keydir = get_datadir_fname("keys"); - if (check_private_dir(keydir, CPD_CREATE)) { + if (check_private_dir(keydir, CPD_CREATE, options->User)) { tor_free(keydir); return -1; }