April 2014 - tor-commits - lists.torproject.org

[tor/master] scan-build: when logging a path length, check build_state.
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 7cd9520ba9713c10ef9f958a977a1d3d8d1a2c4c Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 18 20:40:34 2014 -0400 scan-build: when logging a path length, check build_state. Throughout circuituse, when we log about a circuit, we log its desired path length from build_state. scan-build is irrationally concerned that build_state might be NULL. --- src/or/circuituse.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 75a10ba0..d104306 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -537,7 +537,9 @@ circuit_expire_building(void) "%d guards are live.", TO_ORIGIN_CIRCUIT(victim)->global_identifier, circuit_purpose_to_string(victim->purpose), - TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len, + TO_ORIGIN_CIRCUIT(victim)->build_state ? + TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len : + -1, circuit_state_to_string(victim->state), channel_state_to_string(victim->n_chan->state), num_live_entry_guards(0)); @@ -561,7 +563,9 @@ circuit_expire_building(void) "anyway. %d guards are live.", TO_ORIGIN_CIRCUIT(victim)->global_identifier, circuit_purpose_to_string(victim->purpose), - TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len, + TO_ORIGIN_CIRCUIT(victim)->build_state ? + TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len : + -1, circuit_state_to_string(victim->state), channel_state_to_string(victim->n_chan->state), (long)build_close_ms, @@ -707,7 +711,8 @@ circuit_expire_building(void) * and we have tried to send an INTRODUCE1 cell specifying it. * Thus, if the pending_final_cpath field *is* NULL, then we * want to not spare it. */ - if (TO_ORIGIN_CIRCUIT(victim)->build_state->pending_final_cpath == + if (TO_ORIGIN_CIRCUIT(victim)->build_state && + TO_ORIGIN_CIRCUIT(victim)->build_state->pending_final_cpath == NULL) break; /* fallthrough! */ @@ -753,7 +758,9 @@ circuit_expire_building(void) TO_ORIGIN_CIRCUIT(victim)->has_opened, victim->state, circuit_state_to_string(victim->state), victim->purpose, - TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len); + TO_ORIGIN_CIRCUIT(victim)->build_state ? + TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len : + -1); else log_info(LD_CIRC, "Abandoning circ %u %u (state %d,%d:%s, purpose %d, len %d)", @@ -762,7 +769,9 @@ circuit_expire_building(void) TO_ORIGIN_CIRCUIT(victim)->has_opened, victim->state, circuit_state_to_string(victim->state), victim->purpose, - TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len); + TO_ORIGIN_CIRCUIT(victim)->build_state ? + TO_ORIGIN_CIRCUIT(victim)->build_state->desired_path_len : + -1); circuit_log_path(LOG_INFO,LD_CIRC,TO_ORIGIN_CIRCUIT(victim)); if (victim->purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT)

1 0

[tor/master] scan-build: in cpuworker, initialize tv_start
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 78bc814c049fe67d0b521a288d3516248e110301 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 18 21:12:45 2014 -0400 scan-build: in cpuworker, initialize tv_start scan-build doesn't realize that a request can't be timed at the end unless it's timed at the start, and so it's not possible for us to be subtracting start from end without start being set. Nevertheless, let's not confuse it. --- src/or/cpuworker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 209274d..6b6a68a 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -436,7 +436,7 @@ cpuworker_main(void *data) if (req.task == CPUWORKER_TASK_ONION) { const create_cell_t *cc = &req.create_cell; created_cell_t *cell_out = &rpl.created_cell; - struct timeval tv_start, tv_end; + struct timeval tv_start = {0,0}, tv_end; int n; rpl.timed = req.timed; rpl.started_at = req.started_at;

1 0

[tor/master] scan-build: get_proxy_addrport should always set its outputs
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 895b6789e8b33af180a00d843eb25343bace4a4d Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 18 20:41:40 2014 -0400 scan-build: get_proxy_addrport should always set its outputs When get_proxy_addrport returned PROXY_NONE, it would leave addr/port unset. This is inconsistent, and could (if we used the function in a stupid way) lead to undefined behavior. Bugfix on 5b050a9b0, though I don't think it affects tor-as-it-is. --- src/or/connection.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/or/connection.c b/src/or/connection.c index 2e72e6b..5dbc477 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4814,6 +4814,8 @@ get_proxy_addrport(tor_addr_t *addr, uint16_t *port, int *proxy_type, } } + tor_addr_make_unspec(addr); + *port = 0; *proxy_type = PROXY_NONE; return 0; }

1 0

[tor/master] scan-build: bulletproof last-chance errormsg generation in rendservice.c
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit d4ad254917ffa0dfba371624f72bc9e163645b8e Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 18 21:24:16 2014 -0400 scan-build: bulletproof last-chance errormsg generation in rendservice.c If 'intro' is NULL in these functions, I'm pretty sure that the error message must be set before we hit the end. But scan-build doesn't notice that, and is worried that we'll do a null-pointer dereference in the last-chance errormsg generation. --- src/or/rendservice.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index abd78da..5a81d07 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2041,7 +2041,7 @@ rend_service_decrypt_intro( if (err_msg_out && !err_msg) { tor_asprintf(&err_msg, "unknown INTRODUCE%d error decrypting encrypted part", - (int)(intro->type)); + intro ? (int)(intro->type) : -1); } if (status >= 0) status = -1; @@ -2147,7 +2147,7 @@ rend_service_parse_intro_plaintext( if (err_msg_out && !err_msg) { tor_asprintf(&err_msg, "unknown INTRODUCE%d error parsing encrypted part", - (int)(intro->type)); + intro ? (int)(intro->type) : -1); } if (status >= 0) status = -1;

1 0

[tor/master] scan-build: close stdio FILEs on error in tor-gencert
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 5670e38efb8529d3439b8a160e9f19c4147e01ad Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Apr 19 12:47:58 2014 -0400 scan-build: close stdio FILEs on error in tor-gencert This is harmless, since tor-gencert exits right afterwards, but it's best to clean up after ourselves. --- src/tools/tor-gencert.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tools/tor-gencert.c b/src/tools/tor-gencert.c index d0c30b8..e799df5 100644 --- a/src/tools/tor-gencert.c +++ b/src/tools/tor-gencert.c @@ -302,6 +302,7 @@ load_identity_key(void) if (!identity_key) { log_err(LD_GENERAL, "Couldn't read identity key from %s", identity_key_file); + fclose(f); return 1; } fclose(f); @@ -322,6 +323,7 @@ load_signing_key(void) } if (!(signing_key = PEM_read_PrivateKey(f, NULL, NULL, NULL))) { log_err(LD_GENERAL, "Couldn't read siging key from %s", signing_key_file); + fclose(f); return 1; } fclose(f);

1 0

[tor/master] scan-build: Have clear_pending_onions walk the lists more obviously
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 1b3bddd013dab6d0aa8159e1690d944e226ed77f Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 18 21:17:40 2014 -0400 scan-build: Have clear_pending_onions walk the lists more obviously As it stands, it relies on the fact that onion_queue_entry_remove will magically remove each onionskin from the right list. This patch changes the logic to be more resilient to possible bugs in onion_queue_entry_remove, and less confusing to static analysis tools. --- src/or/onion.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/or/onion.c b/src/or/onion.c index 30b983d..72571b7 100644 --- a/src/or/onion.c +++ b/src/or/onion.c @@ -329,12 +329,14 @@ onion_queue_entry_remove(onion_queue_t *victim) void clear_pending_onions(void) { - onion_queue_t *victim; + onion_queue_t *victim, *next; int i; for (i=0; i<=MAX_ONION_HANDSHAKE_TYPE; i++) { - while ((victim = TOR_TAILQ_FIRST(&ol_list[i]))) { + for (victim = TOR_TAILQ_FIRST(&ol_list[i]); victim; victim = next) { + next = TOR_TAILQ_NEXT(victim,next); onion_queue_entry_remove(victim); } + tor_assert(TOR_TAILQ_EMPTY(&ol_list[i])); } memset(ol_entries, 0, sizeof(ol_entries)); }

1 0

[tor/master] scan-build: truncate tinytest hexified outputs to 1024 bytes.
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 9c9e07963dddff6e11330e9dc8ad7a6d37da4aa4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Apr 19 12:44:31 2014 -0400 scan-build: truncate tinytest hexified outputs to 1024 bytes. scan-build didn't like the unlimited version since we might need to overflow size_t to hexify a string that took up half our address space. (!) --- src/ext/tinytest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/ext/tinytest.c b/src/ext/tinytest.c index 3a8e331..cc054ad 100644 --- a/src/ext/tinytest.c +++ b/src/ext/tinytest.c @@ -478,16 +478,23 @@ tinytest_format_hex_(const void *val_, unsigned long len) const unsigned char *val = val_; char *result, *cp; size_t i; + int ellipses = 0; if (!val) return strdup("null"); - if (!(result = malloc(len*2+1))) + if (len > 1024) { + ellipses = 3; + len = 1024; + } + if (!(result = malloc(len*2+4))) return strdup("<allocation failure>"); cp = result; for (i=0;i<len;++i) { *cp++ = "0123456789ABCDEF"[val[i] >> 4]; *cp++ = "0123456789ABCDEF"[val[i] & 0x0f]; } + while (ellipses--) + *cp++ = '.'; *cp = 0; return result; }

1 0

[tor/master] scan-build: Fix harmless sizeof(ptr) in test_oom.c
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 1800e79ca508e555d43eb8ca36e9544f42c98944 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Apr 19 12:50:17 2014 -0400 scan-build: Fix harmless sizeof(ptr) in test_oom.c We meant to using random bytes to fill a buffer, up to 3000 at a time. Instead we were taking them sizeof(void*) at a time. --- src/test/test_oom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test_oom.c b/src/test/test_oom.c index cc6e532..989ca12 100644 --- a/src/test/test_oom.c +++ b/src/test/test_oom.c @@ -82,8 +82,8 @@ add_bytes_to_buf(generic_buffer_t *buf, size_t n_bytes) char b[3000]; while (n_bytes) { - size_t this_add = n_bytes > sizeof(buf) ? sizeof(buf) : n_bytes; - crypto_rand(b, sizeof(b)); + size_t this_add = n_bytes > sizeof(b) ? sizeof(b) : n_bytes; + crypto_rand(b, this_add); generic_buffer_add(buf, b, this_add); n_bytes -= this_add; }

1 0

[tor/master] scan-build: limit hashtable size so it always fits in SSIZE_MAX
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 4d51dcda2fa75a3841e041ab7c3de325d73e2850 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Apr 19 12:39:14 2014 -0400 scan-build: limit hashtable size so it always fits in SSIZE_MAX scan-build recognizes that in theory there could be a numeric overflow here. This can't numeric overflow can't trigger IRL, since in order to fill a hash table with more than P=402653189 buckets with a reasonable load factor of 0.5, we'd first have P/2 malloced objects to put in it--- and each of those would have to take take at least sizeof(void*) worth of malloc overhead plus sizeof(void*) content, which would run you out of address space anyway on a 32-bit system. --- src/ext/ht.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ext/ht.h b/src/ext/ht.h index e76b4aa..4a68673 100644 --- a/src/ext/ht.h +++ b/src/ext/ht.h @@ -303,14 +303,16 @@ ht_string_hash(const char *s) #define HT_GENERATE(name, type, field, hashfn, eqfn, load, mallocfn, \ reallocfn, freefn) \ + /* Primes that aren't too far from powers of two. We stop at */ \ + /* P=402653189 because P*sizeof(void*) is less than SSIZE_MAX */ \ + /* even on a 32-bit platform. */ \ static unsigned name##_PRIMES[] = { \ 53, 97, 193, 389, \ 769, 1543, 3079, 6151, \ 12289, 24593, 49157, 98317, \ 196613, 393241, 786433, 1572869, \ 3145739, 6291469, 12582917, 25165843, \ - 50331653, 100663319, 201326611, 402653189, \ - 805306457, 1610612741 \ + 50331653, 100663319, 201326611, 402653189 \ }; \ static unsigned name##_N_PRIMES = \ (unsigned)(sizeof(name##_PRIMES)/sizeof(name##_PRIMES[0])); \

1 0

[tor/master] scan-build: avoid undef behaior in tor_inet_pton
by nickm＠torproject.org 25 Apr '14

25 Apr '14

commit 685d450ab3823c578514ce6986d00c6e219abb43 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Apr 19 13:07:30 2014 -0400 scan-build: avoid undef behaior in tor_inet_pton If we had an address of the form "1.2.3.4" and we tried to pass it to tor_inet_pton with AF_INET6, it was possible for our 'eow' pointer to briefly move backwards to the point before the start of the string, before we moved it right back to the start of the string. C doesn't allow that, and though we haven't yet hit a compiler that decided to nuke us in response, it's best to fix. So, be more explicit about requiring there to be a : before any IPv4 address part of the IPv6 address. We would have rejected addresses without a : for not being IPv6 later on anyway. --- src/common/compat.c | 4 +++- src/test/test_addr.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/common/compat.c b/src/common/compat.c index c5945fb..8d816b9 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -2195,8 +2195,10 @@ tor_inet_pton(int af, const char *src, void *dst) else { unsigned byte1,byte2,byte3,byte4; char more; - for (eow = dot-1; eow >= src && TOR_ISDIGIT(*eow); --eow) + for (eow = dot-1; eow > src && TOR_ISDIGIT(*eow); --eow) ; + if (*eow != ':') + return 0; ++eow; /* We use "scanf" because some platform inet_aton()s are too lax diff --git a/src/test/test_addr.c b/src/test/test_addr.c index cee2dcf..50011e6 100644 --- a/src/test/test_addr.c +++ b/src/test/test_addr.c @@ -346,6 +346,9 @@ test_addr_ip6_helpers(void) test_pton6_bad("a:::b:c"); test_pton6_bad(":::a:b:c"); test_pton6_bad("a:b:c:::"); + test_pton6_bad("1.2.3.4"); + test_pton6_bad(":1.2.3.4"); + test_pton6_bad(".2.3.4"); /* test internal checking */ test_external_ip("fbff:ffff::2:7", 0);

1 0