From 08325b58bef83bfed181c493f269ef57477152c0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:26:47 -0400 Subject: [PATCH 01/19] scan-build: Add a check for result from getaddrinfo As documented, getaddrinfo always sets its result when it returns no error. But scan-build doesn't know that, and thinks we might be def --- src/common/address.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/common/address.c b/src/common/address.c index e5930dedca..2825b123da 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -236,7 +236,9 @@ tor_addr_lookup(const char *name, uint16_t family, tor_addr_t *addr) hints.ai_family = family; hints.ai_socktype = SOCK_STREAM; err = sandbox_getaddrinfo(name, NULL, &hints, &res); - if (!err) { + /* The check for 'res' here shouldn't be necessary, but it makes static + * analysis tools happy. */ + if (!err && res) { best = NULL; for (res_p = res; res_p; res_p = res_p->ai_next) { if (family == AF_UNSPEC) { From 41a8930fa1202b882687c0c3a328307b480934b5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:28:46 -0400 Subject: [PATCH 02/19] scan-build: check impossible null-pointer case in buffers.c When maintaining buffer freelists, we don't skip more than there are, so (*chp) can't be null to begin with. scan-build has no way to know that. --- src/or/buffers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 012ced6d32..fb186081cf 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -322,7 +322,7 @@ buf_shrink_freelists(int free_all) chunk_t **chp = &freelists[i].head; chunk_t *chunk; while (n_to_skip) { - if (! (*chp)->next) { + if (!(*chp) || ! (*chp)->next) { log_warn(LD_BUG, "I wanted to skip %d chunks in the freelist for " "%d-byte chunks, but only found %d. (Length %d)", orig_n_to_skip, (int)freelists[i].alloc_size, From d1be2f5cf8afc7d94f4c69081897d7ea3da71298 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:29:51 -0400 Subject: [PATCH 03/19] scan-build: circuit_cpath_support_ntor had a dead initialization We were initializing cpath twice, which doesn't make sense. --- src/or/circuitbuild.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 98fef4c142..6bdbead2eb 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -277,9 +277,9 @@ circuit_rep_hist_note_result(origin_circuit_t *circ) static int circuit_cpath_supports_ntor(const origin_circuit_t *circ) { - crypt_path_t *head = circ->cpath, *cpath = circ->cpath; + crypt_path_t *head, *cpath; - cpath = head; + cpath = head = circ->cpath; do { if (cpath->extend_info && !tor_mem_is_zero( From 0fd0f5f7a9309fb90a6a4d8bad7d6399a45c7cc1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:30:46 -0400 Subject: [PATCH 04/19] scan-build: Avoid crashing on BUG in circuit_get_by_rend_token_and_purpose If we fail in circuit_get_by_rend_token_and_purpose because the circuit has no rend_info, don't try to reference fiends from its rend_info when logging an error. Bugfix on 8b9a2cb68, which is going into Tor 0.2.5.4-alpha. --- src/or/circuitlist.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index b71dc3c13a..24cb9fc8d7 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1261,8 +1261,16 @@ circuit_get_by_rend_token_and_purpose(uint8_t purpose, int is_rend_circ, circ->base_.marked_for_close) return NULL; - if (!circ->rendinfo || - ! bool_eq(circ->rendinfo->is_rend_circ, is_rend_circ) || + if (!circ->rendinfo) { + char *t = tor_strdup(hex_str(token, REND_TOKEN_LEN)); + log_warn(LD_BUG, "Wanted a circuit with %s:%d, but lookup returned a " + "circuit with no rendinfo set.", + safe_str(t), is_rend_circ); + tor_free(t); + return NULL; + } + + if (! bool_eq(circ->rendinfo->is_rend_circ, is_rend_circ) || tor_memneq(circ->rendinfo->rend_token, token, REND_TOKEN_LEN)) { char *t = tor_strdup(hex_str(token, REND_TOKEN_LEN)); log_warn(LD_BUG, "Wanted a circuit with %s:%d, but lookup returned %s:%d", From 69ea4450caee65be56912fd2618c2b95413a0763 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:33:21 -0400 Subject: [PATCH 05/19] scan-build: fix a crash-on-fail possibility in test_policy.c --- src/test/test_policy.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/test_policy.c b/src/test/test_policy.c index d2ba1612de..491c9a21fb 100644 --- a/src/test/test_policy.c +++ b/src/test/test_policy.c @@ -417,8 +417,10 @@ test_dump_exit_policy_to_string(void *arg) done: - SMARTLIST_FOREACH(ri->exit_policy, addr_policy_t *, - entry, addr_policy_free(entry)); + if (ri->exit_policy) { + SMARTLIST_FOREACH(ri->exit_policy, addr_policy_t *, + entry, addr_policy_free(entry)); + } tor_free(ri); tor_free(ep); } From 710649257176a35b28c0ca5f2b823d39e011350c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:33:47 -0400 Subject: [PATCH 06/19] scan-build: Be consistent with a needless check in circuitmux.c In circuitmux_detach_all_circuits, we check whether an HT iterator gives us NULL. That should be impossible for an HT iterator. But our checking it has confused scan-build (justly) into thinking that our later use of HT_NEXT_RMV might not be kosher. I'm taking the coward's route here and strengthening the check. Bugfix on fd31dd44. (Not a real bug though) --- src/or/circuitmux.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 2d05c748ec..52ebfef084 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -412,7 +412,11 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) i = HT_START(chanid_circid_muxinfo_map, cmux->chanid_circid_map); while (i) { to_remove = *i; - if (to_remove) { + + if (! to_remove) { + log_warn(LD_BUG, "Somehow, an HT iterator gave us a NULL pointer."); + break; + } else { /* Find a channel and circuit */ chan = channel_find_by_global_id(to_remove->chan_id); if (chan) { From 7cd9520ba9713c10ef9f958a977a1d3d8d1a2c4c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:40:34 -0400 Subject: [PATCH 07/19] 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 75a10ba0c4..d10430668b 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) From 895b6789e8b33af180a00d843eb25343bace4a4d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:41:40 -0400 Subject: [PATCH 08/19] 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 2e72e6b397..5dbc477283 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; } From 78bc814c049fe67d0b521a288d3516248e110301 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:12:45 -0400 Subject: [PATCH 09/19] 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 209274da64..6b6a68afe5 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; From 1b3bddd013dab6d0aa8159e1690d944e226ed77f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:17:40 -0400 Subject: [PATCH 10/19] 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 30b983d91e..72571b7bd9 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)); } From d4ad254917ffa0dfba371624f72bc9e163645b8e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:24:16 -0400 Subject: [PATCH 11/19] 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 abd78da73a..5a81d07856 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; From 4d51dcda2fa75a3841e041ab7c3de325d73e2850 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:39:14 -0400 Subject: [PATCH 12/19] 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 e76b4aa4d9..4a68673e6e 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])); \ From 9c9e07963dddff6e11330e9dc8ad7a6d37da4aa4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:44:31 -0400 Subject: [PATCH 13/19] 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 3a8e331055..cc054ad340 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(""); cp = result; for (i=0;i> 4]; *cp++ = "0123456789ABCDEF"[val[i] & 0x0f]; } + while (ellipses--) + *cp++ = '.'; *cp = 0; return result; } From 5670e38efb8529d3439b8a160e9f19c4147e01ad Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:47:58 -0400 Subject: [PATCH 14/19] 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 d0c30b8b02..e799df5cad 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); From 1800e79ca508e555d43eb8ca36e9544f42c98944 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:50:17 -0400 Subject: [PATCH 15/19] 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 cc6e532358..989ca1203b 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; } From 78f555a2480b03911e602c2c041a10fd010804b9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:53:57 -0400 Subject: [PATCH 16/19] scan-build: sizeof(ptr*) in a debugging log in ext_orport.c Instead of taking the length of a buffer, we were taking the length of a pointer, so that our debugging log would cover only the first sizeof(void*) bytes of the client nonce. --- src/or/ext_orport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/ext_orport.c b/src/or/ext_orport.c index d5a0fa1ee4..0d28a9199a 100644 --- a/src/or/ext_orport.c +++ b/src/or/ext_orport.c @@ -256,7 +256,7 @@ handle_client_auth_nonce(const char *client_nonce, size_t client_nonce_len, base16_encode(server_nonce_encoded, sizeof(server_nonce_encoded), server_nonce, sizeof(server_nonce)); base16_encode(client_nonce_encoded, sizeof(client_nonce_encoded), - client_nonce, sizeof(client_nonce)); + client_nonce, EXT_OR_PORT_AUTH_NONCE_LEN); log_debug(LD_GENERAL, "server_hash: '%s'\nserver_nonce: '%s'\nclient_nonce: '%s'", From 685d450ab3823c578514ce6986d00c6e219abb43 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 13:07:30 -0400 Subject: [PATCH 17/19] 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 c5945fbd22..8d816b90e7 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 cee2dcf2a0..50011e606b 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); From 3b1f7f75a7efa51ae5549a6413e90066cfe307a8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 13:16:56 -0400 Subject: [PATCH 18/19] scan-build: memarea_strndup() undefined behavior The memarea_strndup() function would have hit undefined behavior by creating an 'end' pointer off the end of a string if it had ever been given an 'n' argument bigger than the length of the memory ares that it's scanning. Fortunately, we never did that except in the unit tests. But it's not a safe behavior to leave lying around. --- src/common/memarea.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/common/memarea.c b/src/common/memarea.c index e2d07fca9e..bcaea0949e 100644 --- a/src/common/memarea.c +++ b/src/common/memarea.c @@ -291,14 +291,11 @@ memarea_strdup(memarea_t *area, const char *s) char * memarea_strndup(memarea_t *area, const char *s, size_t n) { - size_t ln; + size_t ln = 0; char *result; - const char *cp, *end = s+n; tor_assert(n < SIZE_T_CEILING); - for (cp = s; cp < end && *cp; ++cp) + for (ln = 0; ln < n && s[ln]; ++ln) ; - /* cp now points to s+n, or to the 0 in the string. */ - ln = cp-s; result = memarea_alloc(area, ln+1); memcpy(result, s, ln); result[ln]='\0'; From 5470795b834a788ec482f22091d58b27a1999a2d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 25 Apr 2014 01:18:16 -0400 Subject: [PATCH 19/19] Changes file for scan-build fixes (#8793) --- changes/bug8793 | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changes/bug8793 diff --git a/changes/bug8793 b/changes/bug8793 new file mode 100644 index 0000000000..f22c474035 --- /dev/null +++ b/changes/bug8793 @@ -0,0 +1,9 @@ + o Minor bugfixes: + - Fix numerous warnings from the clang "scan-build" static analyzer. + Some of these are programming style issues; some of them are false + positives that indicated awkward code; some are undefined behavior + cases related to constructing (but not using) invalid pointers; + some are assumptions about API behavior; some are using + sizeof(ptr) when sizeof(*ptr) would be correct; and one or two are + genuine bugs that weren't reachable from the rest of the + program. Fixes bug 8793; bugfixes on many, many tor versions.