From 08325b58bef83bfed181c493f269ef57477152c0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:26:47 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/common/address.c b/src/common/address.c index e5930dedc..2825b123d 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) { -- cgit v1.2.3 From 41a8930fa1202b882687c0c3a328307b480934b5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:28:46 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/buffers.c b/src/or/buffers.c index 012ced6d3..fb186081c 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, -- cgit v1.2.3 From d1be2f5cf8afc7d94f4c69081897d7ea3da71298 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:29:51 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 98fef4c14..6bdbead2e 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( -- cgit v1.2.3 From 0fd0f5f7a9309fb90a6a4d8bad7d6399a45c7cc1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:30:46 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index b71dc3c13..24cb9fc8d 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", -- cgit v1.2.3 From 69ea4450caee65be56912fd2618c2b95413a0763 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:33:21 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/test/test_policy.c b/src/test/test_policy.c index d2ba1612d..491c9a21f 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); } -- cgit v1.2.3 From 710649257176a35b28c0ca5f2b823d39e011350c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:33:47 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 2d05c748e..52ebfef08 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) { -- cgit v1.2.3 From 7cd9520ba9713c10ef9f958a977a1d3d8d1a2c4c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:40:34 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 75a10ba0c..d10430668 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) -- cgit v1.2.3 From 895b6789e8b33af180a00d843eb25343bace4a4d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 20:41:40 -0400 Subject: 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(+) (limited to 'src') diff --git a/src/or/connection.c b/src/or/connection.c index 2e72e6b39..5dbc47728 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; } -- cgit v1.2.3 From 78bc814c049fe67d0b521a288d3516248e110301 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:12:45 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 209274da6..6b6a68afe 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; -- cgit v1.2.3 From 1b3bddd013dab6d0aa8159e1690d944e226ed77f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:17:40 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/onion.c b/src/or/onion.c index 30b983d91..72571b7bd 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)); } -- cgit v1.2.3 From d4ad254917ffa0dfba371624f72bc9e163645b8e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 21:24:16 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/rendservice.c b/src/or/rendservice.c index abd78da73..5a81d0785 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; -- cgit v1.2.3 From 4d51dcda2fa75a3841e041ab7c3de325d73e2850 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:39:14 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/ext/ht.h b/src/ext/ht.h index e76b4aa4d..4a68673e6 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])); \ -- cgit v1.2.3 From 9c9e07963dddff6e11330e9dc8ad7a6d37da4aa4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:44:31 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/ext/tinytest.c b/src/ext/tinytest.c index 3a8e33105..cc054ad34 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; } -- cgit v1.2.3 From 5670e38efb8529d3439b8a160e9f19c4147e01ad Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:47:58 -0400 Subject: 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(+) (limited to 'src') diff --git a/src/tools/tor-gencert.c b/src/tools/tor-gencert.c index d0c30b8b0..e799df5ca 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); -- cgit v1.2.3 From 1800e79ca508e555d43eb8ca36e9544f42c98944 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:50:17 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/test/test_oom.c b/src/test/test_oom.c index cc6e53235..989ca1203 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; } -- cgit v1.2.3 From 78f555a2480b03911e602c2c041a10fd010804b9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 12:53:57 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/or/ext_orport.c b/src/or/ext_orport.c index d5a0fa1ee..0d28a9199 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'", -- cgit v1.2.3 From 685d450ab3823c578514ce6986d00c6e219abb43 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 13:07:30 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/common/compat.c b/src/common/compat.c index c5945fbd2..8d816b90e 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 cee2dcf2a..50011e606 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); -- cgit v1.2.3 From 3b1f7f75a7efa51ae5549a6413e90066cfe307a8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 19 Apr 2014 13:16:56 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/common/memarea.c b/src/common/memarea.c index e2d07fca9..bcaea0949 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'; -- cgit v1.2.3