- tor-commits - lists.torproject.org

[tor/master] Merge branch 'cov217_master'
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit e273890b10092b0a482204920606f3d6d0fdc4b9 Merge: 734e860 0317dc2 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 12:57:21 2011 -0400 Merge branch 'cov217_master' changes/cov217_scanf | 5 +++++ src/common/compat_libevent.c | 4 ++-- src/or/config.c | 2 +- src/or/directory.c | 6 +++--- src/or/eventdns.c | 4 ++-- src/or/geoip.c | 4 ++-- src/or/policies.c | 14 +++++++------- src/or/relay.c | 8 ++++---- src/or/routerlist.c | 2 ++ src/or/routerparse.c | 8 ++++---- src/test/test.c | 5 ++++- 11 files changed, 36 insertions(+), 26 deletions(-)

1 0

[tor/master] Replace a "const const" with a "const"
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit da62af6f6b369592609d2da43b82abde0bbccfac Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:11:35 2011 -0400 Replace a "const const" with a "const" Looks like this squeaked in while I was doing a search-and-replace to constify things. Coverity CID 483. --- src/or/config.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index df5cc86..bb335dd 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -2367,7 +2367,7 @@ options_trial_assign(config_line_t *list, int use_defaults, * Called from option_reset() and config_free(). */ static void option_clear(const config_format_t *fmt, or_options_t *options, - const const config_var_t *var) + const config_var_t *var) { void *lvalue = STRUCT_VAR_P(options, var->var_offset); (void)fmt; /* unused */

1 0

[tor/master] Defensive programming: don't crash with broken node_t
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit eca982d3eb1e715394b2c67d8f420bcbe4a8486d Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:43:34 2011 -0400 Defensive programming: don't crash with broken node_t Every node_t has either a routerinfo_t or a routerstatus_t, so every node_t *should* have a nickname. Nonetheless, let's make sure in hex_digest_nickname_matches(). Should quiet CID 434. --- src/or/routerlist.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index f711282..15f643c 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2274,6 +2274,8 @@ hex_digest_nickname_matches(const char *hexdigest, const char *identity_digest, return 0; if (nn_char == '=' || nn_char == '~') { + if (!nickname) + return 0; if (strcasecmp(nn_buf, nickname)) return 0; if (nn_char == '=' && !is_named)

1 0

[tor/master] Don't shadow parameters with local variables
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit 1d18c2deb6d048a8d6f726e6c1b8ccbab4374e32 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:33:07 2011 -0400 Don't shadow parameters with local variables This is a little error-prone when the local has a different type from the parameter, and is very error-prone with both have the same type. Let's not do this. Fixes CID #437,438,439,440,441. --- src/or/directory.c | 6 +++--- src/or/eventdns.c | 4 ++-- src/or/policies.c | 14 +++++++------- src/or/relay.c | 8 ++++---- src/or/routerparse.c | 8 ++++---- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 7ded115..9e1373d 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2592,7 +2592,7 @@ client_likes_consensus(networkstatus_t *v, const char *want_url) * Always return 0. */ static int directory_handle_command_get(dir_connection_t *conn, const char *headers, - const char *body, size_t body_len) + const char *req_body, size_t req_body_len) { size_t dlen; char *url, *url_mem, *header; @@ -2602,8 +2602,8 @@ directory_handle_command_get(dir_connection_t *conn, const char *headers, size_t url_len; /* We ignore the body of a GET request. */ - (void)body; - (void)body_len; + (void)req_body; + (void)req_body_len; log_debug(LD_DIRSERV,"Received GET command."); diff --git a/src/or/eventdns.c b/src/or/eventdns.c index 321b7d7..7fe376b 100644 --- a/src/or/eventdns.c +++ b/src/or/eventdns.c @@ -1831,8 +1831,8 @@ evdns_server_request_respond(struct evdns_server_request *_req, int err) r = sendto(port->socket, req->response, req->response_len, 0, (struct sockaddr*) &req->addr, req->addrlen); if (r<0) { - int err = last_error(port->socket); - if (! error_is_eagain(err)) + int error = last_error(port->socket); + if (! error_is_eagain(error)) return -1; if (port->pending_replies) { diff --git a/src/or/policies.c b/src/or/policies.c index 983df86..1b5408c 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -83,15 +83,15 @@ policy_expand_private(smartlist_t **policy) continue; } for (i = 0; private_nets[i]; ++i) { - addr_policy_t policy; - memcpy(&policy, p, sizeof(addr_policy_t)); - policy.is_private = 0; - policy.is_canonical = 0; - if (tor_addr_parse_mask_ports(private_nets[i], &policy.addr, - &policy.maskbits, &port_min, &port_max)<0) { + addr_policy_t newpolicy; + memcpy(&newpolicy, p, sizeof(addr_policy_t)); + newpolicy.is_private = 0; + newpolicy.is_canonical = 0; + if (tor_addr_parse_mask_ports(private_nets[i], &newpolicy.addr, + &newpolicy.maskbits, &port_min, &port_max)<0) { tor_assert(0); } - smartlist_add(tmp, addr_policy_get_canonical_entry(&policy)); + smartlist_add(tmp, addr_policy_get_canonical_entry(&newpolicy)); } addr_policy_free(p); }); diff --git a/src/or/relay.c b/src/or/relay.c index d647b18..df6d0a8 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2337,13 +2337,13 @@ connection_or_flush_from_first_active_circuit(or_connection_t *conn, int max, /* Calculate the exact time that this cell has spent in the queue. */ if (get_options()->CellStatistics && !CIRCUIT_IS_ORIGIN(circ)) { - struct timeval now; + struct timeval tvnow; uint32_t flushed; uint32_t cell_waiting_time; insertion_time_queue_t *it_queue = queue->insertion_times; - tor_gettimeofday_cached(&now); - flushed = (uint32_t)((now.tv_sec % SECONDS_IN_A_DAY) * 100L + - (uint32_t)now.tv_usec / (uint32_t)10000L); + tor_gettimeofday_cached(&tvnow); + flushed = (uint32_t)((tvnow.tv_sec % SECONDS_IN_A_DAY) * 100L + + (uint32_t)tvnow.tv_usec / (uint32_t)10000L); if (!it_queue || !it_queue->first) { log_info(LD_GENERAL, "Cannot determine insertion time of cell. " "Looks like the CellStatistics option was " diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 67b238e..d1b2cd0 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3512,10 +3512,10 @@ networkstatus_parse_detached_signatures(const char *s, const char *eos) siglist = detached_get_signatures(sigs, flavor); is_duplicate = 0; - SMARTLIST_FOREACH(siglist, document_signature_t *, s, { - if (s->alg == alg && - tor_memeq(id_digest, s->identity_digest, DIGEST_LEN) && - tor_memeq(sk_digest, s->signing_key_digest, DIGEST_LEN)) { + SMARTLIST_FOREACH(siglist, document_signature_t *, dsig, { + if (dsig->alg == alg && + tor_memeq(id_digest, dsig->identity_digest, DIGEST_LEN) && + tor_memeq(sk_digest, dsig->signing_key_digest, DIGEST_LEN)) { is_duplicate = 1; } });

1 0

[tor/master] Replace 4 more sscanf()s with tor_sscanf()
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit a0ae80788cc12284cd63ac678318f95e1238b257 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:26:30 2011 -0400 Replace 4 more sscanf()s with tor_sscanf() For some inexplicable reason, Coverity departs from its usual standards of avoiding false positives here, and warns about all sscanf usage, even when the formatting strings are totally safe. Addresses CID # 447, 446. --- changes/cov217_scanf | 5 +++++ src/common/compat_libevent.c | 4 ++-- src/or/geoip.c | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/changes/cov217_scanf b/changes/cov217_scanf new file mode 100644 index 0000000..368bca8 --- /dev/null +++ b/changes/cov217_scanf @@ -0,0 +1,5 @@ + o Code simplification and refactoring: + - Use tor_sscanf in place of scanf in more places through the + code. This makes us a little more locale-independent, and + should help shut up code-analysis tools that can't tell + a safe sscanf string from a dangerous one. diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index e0c7e3a..c338dd6 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -264,7 +264,7 @@ tor_decode_libevent_version(const char *v) /* Try the new preferred "1.4.11-stable" format. * Also accept "1.4.14b-stable". */ - fields = sscanf(v, "%u.%u.%u%c%c", &major, &minor, &patchlevel, &c, &e); + fields = tor_sscanf(v, "%u.%u.%u%c%c", &major, &minor, &patchlevel, &c, &e); if (fields == 3 || ((fields == 4 || fields == 5 ) && (c == '-' || c == '_')) || (fields == 5 && TOR_ISALPHA(c) && (e == '-' || e == '_'))) { @@ -272,7 +272,7 @@ tor_decode_libevent_version(const char *v) } /* Try the old "1.3e" format. */ - fields = sscanf(v, "%u.%u%c%c", &major, &minor, &c, &extra); + fields = tor_sscanf(v, "%u.%u%c%c", &major, &minor, &c, &extra); if (fields == 3 && TOR_ISALPHA(c)) { return V_OLD(major, minor, c); } else if (fields == 2) { diff --git a/src/or/geoip.c b/src/or/geoip.c index 59490bd..62c7a5c 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -116,10 +116,10 @@ geoip_parse_entry(const char *line) ++line; if (*line == '#') return 0; - if (sscanf(line,"%u,%u,%2s", &low, &high, b) == 3) { + if (tor_sscanf(line,"%u,%u,%2s", &low, &high, b) == 3) { geoip_add_entry(low, high, b); return 0; - } else if (sscanf(line,"\"%u\",\"%u\",\"%2s\",", &low, &high, b) == 3) { + } else if (tor_sscanf(line,"\"%u\",\"%u\",\"%2s\",", &low, &high, b) == 3) { geoip_add_entry(low, high, b); return 0; } else {

1 0

[tor/master] Check return value of crypto_global_init in test.c
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit 0317dc2bb2223dd8a51fc8b54a69765764d094d3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:56:09 2011 -0400 Check return value of crypto_global_init in test.c This will let the unit tests fail in a less mystifying way if the crypto subsystem is b0rken, and will also make Coverity happier. CID # 429. --- src/test/test.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/test/test.c b/src/test/test.c index 1db1546..02d238e 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1318,7 +1318,10 @@ main(int c, const char **v) } options->command = CMD_RUN_UNITTESTS; - crypto_global_init(0, NULL, NULL); + if (crypto_global_init(0, NULL, NULL)) { + printf("Can't initialize crypto subsystem; exiting.\n"); + return 1; + } rep_hist_init(); network_init(); setup_directory();

1 0

[tor/master] Fix insanely large stack_allocation in log_credential_status
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit d25feadebbf05d6fce55cfee1e3c8f928903f543 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 12:36:33 2011 -0400 Fix insanely large stack_allocation in log_credential_status I'm not one to insist on C's miserly stack limits, but allocating a 256K array on the stack is too much even for me. Bugfix on 0.2.1.7-alpha. Found by coverity. Fixes CID # 450. --- changes/cid_450 | 5 +++++ src/common/compat.c | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/changes/cid_450 b/changes/cid_450 new file mode 100644 index 0000000..2045fca --- /dev/null +++ b/changes/cid_450 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Don't stack-allocate the list of supplementary GIDs when we're + about to log them. Stack-allocating NGROUPS_MAX gid_t elements + could take up to 256K, which is way too much stack. Found by + Coverity; CID #450. Bugfix on 0.2.1.7-alpha. diff --git a/src/common/compat.c b/src/common/compat.c index 3965108..9533c11 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -1080,7 +1080,8 @@ log_credential_status(void) /* Read, effective and saved GIDs */ gid_t rgid, egid, sgid; /* Supplementary groups */ - gid_t sup_gids[NGROUPS_MAX + 1]; + gid_t *sup_gids = NULL; + int sup_gids_size; /* Number of supplementary groups */ int ngids; @@ -1126,9 +1127,19 @@ log_credential_status(void) #endif /* log supplementary groups */ - if ((ngids = getgroups(NGROUPS_MAX + 1, sup_gids)) < 0) { + sup_gids_size = 64; + sup_gids = tor_malloc(sizeof(gid_t) * 64); + while ((ngids = getgroups(sup_gids_size, sup_gids)) < 0 && + errno == EINVAL && + sup_gids_size < NGROUPS_MAX) { + sup_gids_size *= 2; + sup_gids = tor_realloc(sup_gids, sizeof(gid_t) * sup_gids_size); + } + + if (ngids < 0) { log_warn(LD_GENERAL, "Error getting supplementary GIDs: %s", strerror(errno)); + tor_free(sup_gids); return -1; } else { int i, retval = 0; @@ -1158,6 +1169,7 @@ log_credential_status(void) tor_free(cp); }); smartlist_free(elts); + tor_free(sup_gids); return retval; }

1 0

[tor/master] Fix a rare memory leak in rend_cache_store
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit 46297bc7bd86826fa79195f36059ce408ef45b6c Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 11:52:39 2011 -0400 Fix a rare memory leak in rend_cache_store When we rejected a descriptor for not being the one we wanted, we were letting the parsed descriptor go out of scope. Found by Coverity; CID # 30. Bugfix on 0.2.1.26. (No changes file yet, since this is not in any 0.2.1.x release.) --- src/or/rendcommon.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c index 8727a70..e0c101e 100644 --- a/src/or/rendcommon.c +++ b/src/or/rendcommon.c @@ -1077,6 +1077,7 @@ rend_cache_store(const char *desc, size_t desc_len, int published, log_warn(LD_REND, "Received service descriptor for service ID %s; " "expected descriptor for service ID %s.", query, safe_str(service_id)); + rend_service_descriptor_free(parsed); return -2; } now = time(NULL);

1 0

[tor/master] Add a changelog entry for cid30 fix.
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit 359a30d52e01e6413a052f03b7067c40b10336d2 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 12:46:26 2011 -0400 Add a changelog entry for cid30 fix. --- changes/memleak_rendcache | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/changes/memleak_rendcache b/changes/memleak_rendcache new file mode 100644 index 0000000..93b1f61 --- /dev/null +++ b/changes/memleak_rendcache @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Fix a memory leak when receiving a descriptor for a hidden + service we didn't ask for. Found by Coverity; CID#30. Bugfix on + 0.2.2.26-beta.

1 0

[tor/master] Use strlcpy in create_unix_sockaddr()
by nickm＠torproject.org 01 Jul '11

01 Jul '11

commit 959da6b7f2b5ed63426fd12a9046ac06033f6db1 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jul 1 12:06:54 2011 -0400 Use strlcpy in create_unix_sockaddr() Using strncpy meant that if listenaddress were ever >= sizeof(sockaddr_un.sun_path), we would fail to nul-terminate sun_path. This isn't a big deal: we never read sun_path, and the kernel is smart enough to reject the sockaddr_un if it isn't nul-terminated. Nonetheless, it's a dumb failure mode. Instead, we should reject addresses that don't fit in sockaddr_un.sun_path. Coverity found this; it's CID 428. Bugfix on 0.2.0.3-alpha. --- changes/cid_428 | 5 +++++ src/or/connection.c | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletions(-) diff --git a/changes/cid_428 b/changes/cid_428 new file mode 100644 index 0000000..cb0fc8c --- /dev/null +++ b/changes/cid_428 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Always NUL-terminate the sun_path field of a sockaddr_un before + passing it to the kernel. (Not a security issue: kernels are + smart enough to reject bad sockaddr_uns.) Found by Coverity; CID + # 428. Bugfix on Tor 0.2.0.3-alpha. diff --git a/src/or/connection.c b/src/or/connection.c index 4869a24..2897fe1 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -804,7 +804,13 @@ create_unix_sockaddr(const char *listenaddress, char **readable_address, sockaddr = tor_malloc_zero(sizeof(struct sockaddr_un)); sockaddr->sun_family = AF_UNIX; - strncpy(sockaddr->sun_path, listenaddress, sizeof(sockaddr->sun_path)); + if (strlcpy(sockaddr->sun_path, listenaddress, sizeof(sockaddr->sun_path)) + >= sizeof(sockaddr->sun_path)) { + log_warn(LD_CONFIG, "Unix socket path '%s' is too long to fit.", + escaped(listenaddress)); + tor_free(sockaddr); + return NULL; + } if (readable_address) *readable_address = tor_strdup(listenaddress);

1 0