May 2011 - tor-commits - lists.torproject.org

[tor/master] CONN_LOG_PROTECT()'s first argument may not be 0
by nickm＠torproject.org 09 May '11

09 May '11

commit 8ebb3ce6e27c104e35d65662c04d23795f2b5605 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 03:24:04 2011 +0200 CONN_LOG_PROTECT()'s first argument may not be 0 Make that explicit by adding an assert and removing a null-check. All of its callers currently depend on the argument being non-null anyway. Silences a few clang complaints. --- src/or/or.h | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index 7d354c8..f693ad9 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3203,7 +3203,9 @@ typedef enum buildtimeout_set_event_t { */ #define CONN_LOG_PROTECT(conn, stmt) \ STMT_BEGIN \ - int _log_conn_is_control = (conn && conn->type == CONN_TYPE_CONTROL); \ + int _log_conn_is_control; \ + tor_assert(conn); \ + _log_conn_is_control = (conn->type == CONN_TYPE_CONTROL); \ if (_log_conn_is_control) \ disable_control_logging(); \ STMT_BEGIN stmt; STMT_END; \

1 0

[tor/master] Appease clang - and my tortured mind
by nickm＠torproject.org 09 May '11

09 May '11

commit 80e57af50fe06ac76733ee99644c5efe797b2b8c Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 03:01:25 2011 +0200 Appease clang - and my tortured mind This possible div by 0 warning from clang's analyzer was quite fun to track down. Turns out the current behaviour is safe. --- src/or/circuitbuild.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 0e033a5..2189e0e 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -559,7 +559,9 @@ circuit_build_times_create_histogram(circuit_build_times_t *cbt, * Return the Pareto start-of-curve parameter Xm. * * Because we are not a true Pareto curve, we compute this as the - * weighted average of the N=3 most frequent build time bins. + * weighted average of the N most frequent build time bins. N is either + * 1 if we don't have enough circuit build time data collected, or + * determined by the consensus parameter cbtnummodes (default 3). */ static build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt) @@ -572,6 +574,9 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) int n=0; int num_modes = circuit_build_times_default_num_xm_modes(); + tor_assert(nbins > 0); + tor_assert(num_modes > 0); + // Only use one mode if < 1000 buildtimes. Not enough data // for multiple. if (cbt->total_build_times < CBT_NCIRCUITS_TO_OBSERVE) @@ -579,6 +584,7 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) nth_max_bin = (build_time_t*)tor_malloc_zero(num_modes*sizeof(build_time_t)); + /* Determine the N most common build times */ for (i = 0; i < nbins; i++) { if (histogram[i] >= histogram[nth_max_bin[0]]) { nth_max_bin[0] = i; @@ -600,6 +606,10 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) histogram[nth_max_bin[n]]); } + /* The following assert is safe, because we don't get called when we + * haven't observed at least CBT_MIN_MIN_CIRCUITS_TO_OBSERVE circuits. */ + tor_assert(bin_counts > 0); + ret /= bin_counts; tor_free(histogram); tor_free(nth_max_bin);

1 0

[tor/master] Add an assert to un-confuse clang's analyzer
by nickm＠torproject.org 09 May '11

09 May '11

commit 58a16a4d6f47728e029cc8380604bda262a40d30 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 02:20:29 2011 +0200 Add an assert to un-confuse clang's analyzer The analyzer assumed that bootstrap_percent could be less than 0 when we call control_event_bootstrap_problem(), which would mean we're calling log_fn() with undefined values. The assert makes it clear this can't happen. --- src/or/control.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/or/control.c b/src/or/control.c index 28780d2..926a465 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -3931,6 +3931,9 @@ control_event_bootstrap_problem(const char *warn, int reason) char buf[BOOTSTRAP_MSG_LEN]; const char *recommendation = "ignore"; + /* bootstrap_percent must not be in "undefined" state here. */ + tor_assert(status >= 0); + if (bootstrap_percent == 100) return; /* already bootstrapped; nothing to be done here. */

1 0

[tor/maint-0.2.2] Remove a duplicated line, found by clang
by nickm＠torproject.org 09 May '11

09 May '11

commit d7d25558fafbff15fb10c6e226824f9f0f6d99c7 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 04:45:48 2011 +0200 Remove a duplicated line, found by clang --- src/or/connection.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 6e7bbd5..fc2097f 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -1240,7 +1240,7 @@ connection_connect(connection_t *conn, const char *address, { int s, inprogress = 0; char addrbuf[256]; - struct sockaddr *dest_addr = (struct sockaddr*) addrbuf; + struct sockaddr *dest_addr; socklen_t dest_addr_len; or_options_t *options = get_options(); int protocol_family;

1 0

[tor/maint-0.2.2] Merge branch 'clang_fixes' into maint-0.2.2
by nickm＠torproject.org 09 May '11

09 May '11

commit 0b3166fffa0e5e19c63fd95538f87d634444d28c Merge: 330116f 9da4e25 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon May 9 13:20:04 2011 -0400 Merge branch 'clang_fixes' into maint-0.2.2 changes/dirvote_null_deref | 4 ++++ changes/mdesc_null_deref | 5 +++++ src/or/circuitbuild.c | 12 +++++++++++- src/or/connection.c | 2 +- src/or/connection_or.c | 3 +-- src/or/control.c | 5 ++++- src/or/dirvote.c | 6 +++--- src/or/microdesc.c | 2 +- src/or/or.h | 4 +++- 9 files changed, 33 insertions(+), 10 deletions(-)

1 0

[tor/maint-0.2.2] Remove some dead code, found by clang
by nickm＠torproject.org 09 May '11

09 May '11

commit 9da4e25183317ea87d1e7f14e9b13e0530ea0240 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 04:50:15 2011 +0200 Remove some dead code, found by clang --- src/or/connection_or.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 4b932ec..c6049d5 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1414,9 +1414,8 @@ connection_or_send_netinfo(or_connection_t *conn) len = append_address_to_payload(out, &my_addr); if (len < 0) return -1; - out += len; } else { - *out++ = 0; + *out = 0; } connection_or_write_cell_to_buf(&cell, conn);

1 0

[tor/maint-0.2.2] Fix potential null pointer deref during dirvote
by nickm＠torproject.org 09 May '11

09 May '11

commit 1c668540fef93d71e72cfed73955ee705f27163e Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 04:38:55 2011 +0200 Fix potential null pointer deref during dirvote Found by using clang's analyzer. --- changes/dirvote_null_deref | 4 ++++ src/or/dirvote.c | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/changes/dirvote_null_deref b/changes/dirvote_null_deref new file mode 100644 index 0000000..65dc519 --- /dev/null +++ b/changes/dirvote_null_deref @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Fix a potential null-pointer dereference while computing a consensus. + Bugfix on tor-0.2.0.3-alpha, found with the help of clang's analyzer. + diff --git a/src/or/dirvote.c b/src/or/dirvote.c index db2eaf0..750c649 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -444,9 +444,9 @@ compute_routerstatus_consensus(smartlist_t *votes, int consensus_method, if (cur && !compare_vote_rs(cur, rs)) { ++cur_n; } else { - if (cur_n > most_n || - (cur && cur_n == most_n && - cur->status.published_on > most_published)) { + if (cur && (cur_n > most_n || + (cur_n == most_n && + cur->status.published_on > most_published))) { most = cur; most_n = cur_n; most_published = cur->status.published_on;

1 0

[tor/maint-0.2.2] Appease clang - and my tortured mind
by nickm＠torproject.org 09 May '11

09 May '11

commit 80e57af50fe06ac76733ee99644c5efe797b2b8c Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 03:01:25 2011 +0200 Appease clang - and my tortured mind This possible div by 0 warning from clang's analyzer was quite fun to track down. Turns out the current behaviour is safe. --- src/or/circuitbuild.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 0e033a5..2189e0e 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -559,7 +559,9 @@ circuit_build_times_create_histogram(circuit_build_times_t *cbt, * Return the Pareto start-of-curve parameter Xm. * * Because we are not a true Pareto curve, we compute this as the - * weighted average of the N=3 most frequent build time bins. + * weighted average of the N most frequent build time bins. N is either + * 1 if we don't have enough circuit build time data collected, or + * determined by the consensus parameter cbtnummodes (default 3). */ static build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt) @@ -572,6 +574,9 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) int n=0; int num_modes = circuit_build_times_default_num_xm_modes(); + tor_assert(nbins > 0); + tor_assert(num_modes > 0); + // Only use one mode if < 1000 buildtimes. Not enough data // for multiple. if (cbt->total_build_times < CBT_NCIRCUITS_TO_OBSERVE) @@ -579,6 +584,7 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) nth_max_bin = (build_time_t*)tor_malloc_zero(num_modes*sizeof(build_time_t)); + /* Determine the N most common build times */ for (i = 0; i < nbins; i++) { if (histogram[i] >= histogram[nth_max_bin[0]]) { nth_max_bin[0] = i; @@ -600,6 +606,10 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) histogram[nth_max_bin[n]]); } + /* The following assert is safe, because we don't get called when we + * haven't observed at least CBT_MIN_MIN_CIRCUITS_TO_OBSERVE circuits. */ + tor_assert(bin_counts > 0); + ret /= bin_counts; tor_free(histogram); tor_free(nth_max_bin);

1 0

[tor/maint-0.2.2] Fix a potential null deref when rebuilding md cache
by nickm＠torproject.org 09 May '11

09 May '11

commit 1827e60976d54d1917dfc54bdf62b4818662ac12 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 04:23:35 2011 +0200 Fix a potential null deref when rebuilding md cache Issue discovered using clang's static analyzer --- changes/mdesc_null_deref | 5 +++++ src/or/microdesc.c | 2 +- 2 files changed, 6 insertions(+), 1 deletions(-) diff --git a/changes/mdesc_null_deref b/changes/mdesc_null_deref new file mode 100644 index 0000000..30f0280 --- /dev/null +++ b/changes/mdesc_null_deref @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Avoid a possible null-pointer dereference when rebuilding the mdesc + cache without actually having any descriptors to cache. Bugfix on + 0.2.2.6-alpha. Issue discovered using clang's static analyzer. + diff --git a/src/or/microdesc.c b/src/or/microdesc.c index 73d2285..5740c40 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -423,7 +423,7 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force) cache->journal_len = 0; cache->bytes_dropped = 0; - new_size = (int)cache->cache_content->size; + new_size = cache->cache_content ? (int)cache->cache_content->size : 0; log_info(LD_DIR, "Done rebuilding microdesc cache. " "Saved %d bytes; %d still used.", orig_size-new_size, new_size);

1 0

[tor/maint-0.2.2] CONN_LOG_PROTECT()'s first argument may not be 0
by nickm＠torproject.org 09 May '11

09 May '11

commit 8ebb3ce6e27c104e35d65662c04d23795f2b5605 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Tue Apr 26 03:24:04 2011 +0200 CONN_LOG_PROTECT()'s first argument may not be 0 Make that explicit by adding an assert and removing a null-check. All of its callers currently depend on the argument being non-null anyway. Silences a few clang complaints. --- src/or/or.h | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index 7d354c8..f693ad9 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3203,7 +3203,9 @@ typedef enum buildtimeout_set_event_t { */ #define CONN_LOG_PROTECT(conn, stmt) \ STMT_BEGIN \ - int _log_conn_is_control = (conn && conn->type == CONN_TYPE_CONTROL); \ + int _log_conn_is_control; \ + tor_assert(conn); \ + _log_conn_is_control = (conn->type == CONN_TYPE_CONTROL); \ if (_log_conn_is_control) \ disable_control_logging(); \ STMT_BEGIN stmt; STMT_END; \

1 0