commit 5927ed8d3324c39fd8aa3d496d119b37b97a1d73 Author: Nick Mathewson nickm@torproject.org Date: Tue Sep 6 12:35:37 2016 -0400

checkSpace.pl now forbids more identifiers.

The functions it warns about are: assert, memcmp, strcat, strcpy, sprintf, malloc, free, realloc, strdup, strndup, calloc.

Also, fix a few lingering instances of these in the code. Use other conventions to indicate _intended_ use of assert and malloc/realloc/etc. --- scripts/maint/checkSpace.pl | 19 +++++++++++++++++++ src/common/backtrace.c | 4 ++-- src/common/compat.c | 2 +- src/common/container.h | 2 +- src/common/log.c | 16 +++++++++------- src/common/util.c | 6 +++--- src/common/util.h | 10 +++++++++- src/or/channel.c | 8 ++++---- src/or/channel.h | 4 ++-- src/or/channeltls.c | 2 +- src/or/shared_random.c | 4 ++-- src/test/bench.c | 4 ++-- src/test/test-memwipe.c | 13 +++++++------ src/test/test_addr.c | 8 ++++---- src/test/test_crypto.c | 2 +- src/test/test_dir.c | 4 ++-- 16 files changed, 69 insertions(+), 39 deletions(-)

diff --git a/scripts/maint/checkSpace.pl b/scripts/maint/checkSpace.pl index 9062811..e90f5b3 100755 --- a/scripts/maint/checkSpace.pl +++ b/scripts/maint/checkSpace.pl @@ -156,6 +156,25 @@ for $fn (@ARGV) { $in_func_head = 0; } } + + ## Check for forbidden functions except when they are + # explicitly permitted + if (/\bassert(/ && not /assert OK/) { + print "assert :$fn:$. (use tor_assert)\n"; + } + if (/\bmemcmp(/ && not /memcmp OK/) { + print "memcmp :$fn:$. (use {tor,fast}_mem{eq,neq,cmp}\n"; + } + # always forbidden. + if (not / OVERRIDE /) { + if (/\bstrcat(/ or /\bstrcpy(/ or /\bsprintf(/) { + print "$& :$fn:$.\n"; + } + if (/\bmalloc(/ or /\bfree(/ or /\brealloc(/ or + /\bstrdup(/ or /\bstrndup(/ or /\bcalloc(/) { + print "$& :$fn:$. (use tor_malloc, tor_free, etc)\n"; + } + } } } ## Warn if the file doesn't end with a blank line. diff --git a/src/common/backtrace.c b/src/common/backtrace.c index 2841281..81e04e9 100644 --- a/src/common/backtrace.c +++ b/src/common/backtrace.c @@ -117,7 +117,7 @@ log_backtrace(int severity, int domain, const char *msg) for (i=0; i < depth; ++i) { tor_log(severity, domain, " %s", symbols[i]); } - free(symbols); + raw_free(symbols);

done: tor_mutex_release(&cb_buf_mutex); @@ -190,7 +190,7 @@ install_bt_handler(void) size_t depth = backtrace(cb_buf, MAX_DEPTH); symbols = backtrace_symbols(cb_buf, (int) depth); if (symbols) - free(symbols); + raw_free(symbols); }

return rv; diff --git a/src/common/compat.c b/src/common/compat.c index 4614ef9..5385bd8 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -2350,7 +2350,7 @@ make_path_absolute(char *fname) /* We don't want to assume that tor_free can free a string allocated * with malloc. On failure, return fname (it's better than nothing). */ char *absfname = tor_strdup(absfname_malloced ? absfname_malloced : fname); - if (absfname_malloced) free(absfname_malloced); + if (absfname_malloced) raw_free(absfname_malloced);

return absfname; #else diff --git a/src/common/container.h b/src/common/container.h index 92ad3f5..71495b6 100644 --- a/src/common/container.h +++ b/src/common/container.h @@ -526,7 +526,7 @@ void* strmap_remove_lc(strmap_t *map, const char *key); return (valtype*)digestmap_remove((digestmap_t*)map, key); \ } \ ATTR_UNUSED static inline void \ - prefix##free(maptype *map, void (*free_val)(void*)) \ + prefix##f##ree(maptype *map, void (*free_val)(void*)) \ { \ digestmap_free((digestmap_t*)map, free_val); \ } \ diff --git a/src/common/log.c b/src/common/log.c index 71b6790..56adc77 100644 --- a/src/common/log.c +++ b/src/common/log.c @@ -47,6 +47,8 @@ #define TRUNCATED_STR_LEN 14 /** @} */

+#define raw_assert(x) assert(x) // assert OK + /** Information for a single logfile; only used in log.c */ typedef struct logfile_t { struct logfile_t *next; /**< Next logfile_t in the linked list. */ @@ -75,7 +77,7 @@ sev_to_string(int severity) case LOG_ERR: return "err"; default: /* Call assert, not tor_assert, since tor_assert * calls log on failure. */ - assert(0); return "UNKNOWN"; // LCOV_EXCL_LINE + raw_assert(0); return "UNKNOWN"; // LCOV_EXCL_LINE } }

@@ -95,7 +97,7 @@ should_log_function_name(log_domain_mask_t domain, int severity) return (domain & (LD_BUG|LD_NOFUNCNAME)) == LD_BUG; default: /* Call assert, not tor_assert, since tor_assert calls log on failure. */ - assert(0); return 0; // LCOV_EXCL_LINE + raw_assert(0); return 0; // LCOV_EXCL_LINE } }

@@ -293,7 +295,7 @@ format_msg(char *buf, size_t buf_len, char *end_of_prefix; char *buf_end;

- assert(buf_len >= 16); /* prevent integer underflow and general stupidity */ + raw_assert(buf_len >= 16); /* prevent integer underflow and stupidity */ buf_len -= 2; /* subtract 2 characters so we have room for \n\0 */ buf_end = buf+buf_len; /* point *after* the last char we can write to */

@@ -482,12 +484,12 @@ logv,(int severity, log_domain_mask_t domain, const char *funcname, int callbacks_deferred = 0;

/* Call assert, not tor_assert, since tor_assert calls log on failure. */ - assert(format); + raw_assert(format); /* check that severity is sane. Overrunning the masks array leads to * interesting and hard to diagnose effects */ - assert(severity >= LOG_ERR && severity <= LOG_DEBUG); + raw_assert(severity >= LOG_ERR && severity <= LOG_DEBUG); /* check that we've initialised the log mutex before we try to lock it */ - assert(log_mutex_initialized); + raw_assert(log_mutex_initialized); LOCK_LOGS();

if ((! (domain & LD_NOCB)) && pending_cb_messages @@ -658,7 +660,7 @@ tor_log_update_sigsafe_err_fds(void) if (!found_real_stderr && int_array_contains(sigsafe_log_fds, n_sigsafe_log_fds, STDOUT_FILENO)) { /* Don't use a virtual stderr when we're also logging to stdout. */ - assert(n_sigsafe_log_fds >= 2); /* Don't use assert inside log functions*/ + raw_assert(n_sigsafe_log_fds >= 2); /* Don't tor_assert inside log fns */ sigsafe_log_fds[0] = sigsafe_log_fds[--n_sigsafe_log_fds]; }

diff --git a/src/common/util.c b/src/common/util.c index c7dd2a8..211ed7f 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -147,7 +147,7 @@ tor_malloc_(size_t size DMALLOC_PARAMS) #ifdef USE_DMALLOC result = dmalloc_malloc(file, line, size, DMALLOC_FUNC_MALLOC, 0, 0); #else - result = malloc(size); + result = raw_malloc(size); #endif

if (PREDICT_UNLIKELY(result == NULL)) { @@ -246,7 +246,7 @@ tor_realloc_(void *ptr, size_t size DMALLOC_PARAMS) #ifdef USE_DMALLOC result = dmalloc_realloc(file, line, ptr, size, DMALLOC_FUNC_REALLOC, 0); #else - result = realloc(ptr, size); + result = raw_realloc(ptr, size); #endif

if (PREDICT_UNLIKELY(result == NULL)) { @@ -285,7 +285,7 @@ tor_strdup_(const char *s DMALLOC_PARAMS) #ifdef USE_DMALLOC duplicate = dmalloc_strdup(file, line, s, 0); #else - duplicate = strdup(s); + duplicate = raw_strdup(s); #endif if (PREDICT_UNLIKELY(duplicate == NULL)) { /* LCOV_EXCL_START */ diff --git a/src/common/util.h b/src/common/util.h index 7a6203a..57605cc 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -82,7 +82,7 @@ extern int dmalloc_free(const char *file, const int line, void *pnt, */ #define tor_free(p) STMT_BEGIN \ if (PREDICT_LIKELY((p)!=NULL)) { \ - free(p); \ + raw_free(p); \ (p)=NULL; \ } \ STMT_END @@ -99,6 +99,14 @@ extern int dmalloc_free(const char *file, const int line, void *pnt, #define tor_memdup(s, n) tor_memdup_(s, n DMALLOC_ARGS) #define tor_memdup_nulterm(s, n) tor_memdup_nulterm_(s, n DMALLOC_ARGS)

+/* Aliases for the underlying system malloc/realloc/free. Only use + * them to indicate "I really want the underlying system function, I know + * what I'm doing." */ +#define raw_malloc malloc +#define raw_realloc realloc +#define raw_free free +#define raw_strdup strdup + void tor_log_mallinfo(int severity);

/** Return the offset of <b>member</b> within the type <b>tp</b>, in bytes */ diff --git a/src/or/channel.c b/src/or/channel.c index 87fa721..6a78b21 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -838,7 +838,7 @@ channel_free(channel_t *chan) }

/* Call a free method if there is one */ - if (chan->free) chan->free(chan); + if (chan->free_fn) chan->free_fn(chan);

channel_clear_remote_end(chan);

@@ -878,7 +878,7 @@ channel_listener_free(channel_listener_t *chan_l) tor_assert(!(chan_l->registered));

/* Call a free method if there is one */ - if (chan_l->free) chan_l->free(chan_l); + if (chan_l->free_fn) chan_l->free_fn(chan_l);

/* * We're in CLOSED or ERROR, so the incoming channel queue is already @@ -916,7 +916,7 @@ channel_force_free(channel_t *chan) }

/* Call a free method if there is one */ - if (chan->free) chan->free(chan); + if (chan->free_fn) chan->free_fn(chan);

channel_clear_remote_end(chan);

@@ -958,7 +958,7 @@ channel_listener_force_free(channel_listener_t *chan_l) chan_l);

/* Call a free method if there is one */ - if (chan_l->free) chan_l->free(chan_l); + if (chan_l->free_fn) chan_l->free_fn(chan_l);

/* * The incoming list just gets emptied and freed; we request close on diff --git a/src/or/channel.h b/src/or/channel.h index 78e1b71..a711b56 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -90,7 +90,7 @@ struct channel_s { /* Methods implemented by the lower layer */

/** Free a channel */ - void (*free)(channel_t *); + void (*free_fn)(channel_t *); /** Close an open channel */ void (*close)(channel_t *); /** Describe the transport subclass for this channel */ @@ -273,7 +273,7 @@ struct channel_listener_s { /* Methods implemented by the lower layer */

/** Free a channel */ - void (*free)(channel_listener_t *); + void (*free_fn)(channel_listener_t *); /** Close an open channel */ void (*close)(channel_listener_t *); /** Describe the transport subclass for this channel */ diff --git a/src/or/channeltls.c b/src/or/channeltls.c index a62f80e..9c2411e 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -117,7 +117,7 @@ channel_tls_common_init(channel_tls_t *tlschan) chan->state = CHANNEL_STATE_OPENING; chan->close = channel_tls_close_method; chan->describe_transport = channel_tls_describe_transport_method; - chan->free = channel_tls_free_method; + chan->free_fn = channel_tls_free_method; chan->get_overhead_estimate = channel_tls_get_overhead_estimate_method; chan->get_remote_addr = channel_tls_get_remote_addr_method; chan->get_remote_descr = channel_tls_get_remote_descr_method; diff --git a/src/or/shared_random.c b/src/or/shared_random.c index 19564f5..e672a41 100644 --- a/src/or/shared_random.c +++ b/src/or/shared_random.c @@ -578,8 +578,8 @@ commit_is_authoritative(const sr_commit_t *commit, tor_assert(commit); tor_assert(voter_key);

- return !memcmp(commit->rsa_identity, voter_key, - sizeof(commit->rsa_identity)); + return fast_memeq(commit->rsa_identity, voter_key, + sizeof(commit->rsa_identity)); }

/* Decide if the newly received <b>commit</b> should be kept depending on diff --git a/src/test/bench.c b/src/test/bench.c index f1cf715..f373019 100644 --- a/src/test/bench.c +++ b/src/test/bench.c @@ -557,7 +557,7 @@ bench_dh(void) dh_b, dh_pubkey_a, sizeof(dh_pubkey_a), secret_b, sizeof(secret_b)); tor_assert(slen_a == slen_b); - tor_assert(!memcmp(secret_a, secret_b, slen_a)); + tor_assert(fast_memeq(secret_a, secret_b, slen_a)); crypto_dh_free(dh_a); crypto_dh_free(dh_b); } @@ -595,7 +595,7 @@ bench_ecdh_impl(int nid, const char *name) NULL);

tor_assert(slen_a == slen_b); - tor_assert(!memcmp(secret_a, secret_b, slen_a)); + tor_assert(fast_memeq(secret_a, secret_b, slen_a)); EC_KEY_free(dh_a); EC_KEY_free(dh_b); } diff --git a/src/test/test-memwipe.c b/src/test/test-memwipe.c index c28d505..2d40283 100644 --- a/src/test/test-memwipe.c +++ b/src/test/test-memwipe.c @@ -5,6 +5,7 @@

#include "crypto.h" #include "compat.h" +#include "util.h"

static unsigned fill_a_buffer_memset(void) __attribute__((noinline)); static unsigned fill_a_buffer_memwipe(void) __attribute__((noinline)); @@ -98,29 +99,29 @@ static char *heap_buf = NULL; static unsigned fill_heap_buffer_memset(void) { - char *buf = heap_buf = malloc(BUF_LEN); + char *buf = heap_buf = raw_malloc(BUF_LEN); FILL_BUFFER_IMPL() memset(buf, 0, BUF_LEN); - free(buf); + raw_free(buf); return sum; }

static unsigned fill_heap_buffer_memwipe(void) { - char *buf = heap_buf = malloc(BUF_LEN); + char *buf = heap_buf = raw_malloc(BUF_LEN); FILL_BUFFER_IMPL() memwipe(buf, 0, BUF_LEN); - free(buf); + raw_free(buf); return sum; }

static unsigned fill_heap_buffer_nothing(void) { - char *buf = heap_buf = malloc(BUF_LEN); + char *buf = heap_buf = raw_malloc(BUF_LEN); FILL_BUFFER_IMPL() - free(buf); + raw_free(buf); return sum; }

diff --git a/src/test/test_addr.c b/src/test/test_addr.c index dcecb0b..c8a9e6d 100644 --- a/src/test/test_addr.c +++ b/src/test/test_addr.c @@ -81,7 +81,7 @@ test_addr_basic(void *arg) #define test_op_ip6_(a,op,b,e1,e2) \ STMT_BEGIN \ tt_assert_test_fmt_type(a,b,e1" "#op" "e2,struct in6_addr*, \ - (memcmp(val1_->s6_addr, val2_->s6_addr, 16) op 0), \ + (fast_memcmp(val1_->s6_addr, val2_->s6_addr, 16) op 0), \ char *, "%s", \ { char *cp; \ cp = print_ = tor_malloc(64); \ @@ -1037,17 +1037,17 @@ test_addr_make_null(void *data) (void) data; /* Ensure that before tor_addr_make_null, addr != 0's */ memset(addr, 1, sizeof(*addr)); - tt_int_op(memcmp(addr, zeros, sizeof(*addr)), OP_NE, 0); + tt_int_op(fast_memcmp(addr, zeros, sizeof(*addr)), OP_NE, 0); /* Test with AF == AF_INET */ zeros->family = AF_INET; tor_addr_make_null(addr, AF_INET); - tt_int_op(memcmp(addr, zeros, sizeof(*addr)), OP_EQ, 0); + tt_int_op(fast_memcmp(addr, zeros, sizeof(*addr)), OP_EQ, 0); tt_str_op(tor_addr_to_str(buf, addr, sizeof(buf), 0), OP_EQ, "0.0.0.0"); /* Test with AF == AF_INET6 */ memset(addr, 1, sizeof(*addr)); zeros->family = AF_INET6; tor_addr_make_null(addr, AF_INET6); - tt_int_op(memcmp(addr, zeros, sizeof(*addr)), OP_EQ, 0); + tt_int_op(fast_memcmp(addr, zeros, sizeof(*addr)), OP_EQ, 0); tt_str_op(tor_addr_to_str(buf, addr, sizeof(buf), 0), OP_EQ, "::"); done: tor_free(addr); diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 542512b..9cae1e8 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -2258,7 +2258,7 @@ test_crypto_ed25519_simple(void *arg) tt_int_op(0, OP_EQ, ed25519_sign(&manual_sig, (uint8_t *)prefixed_msg, strlen(prefixed_msg), &kp1)); tor_free(prefixed_msg); - tt_assert(!memcmp(sig1.sig, manual_sig.sig, sizeof(sig1.sig))); + tt_assert(fast_memeq(sig1.sig, manual_sig.sig, sizeof(sig1.sig)));

/* Test that prefixed checksig verifies it properly. */ tt_int_op(0, OP_EQ, ed25519_checksig_prefixed(&sig1, msg, msg_len, diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 5d9ae3c..bfd89a9 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -2052,9 +2052,9 @@ test_a_networkstatus(

tt_int_op(4,OP_EQ, smartlist_len(con->voters)); /*3 voters, 1 legacy key.*/ /* The voter id digests should be in this order. */ - tt_assert(memcmp(cert2->cache_info.identity_digest, + tt_assert(fast_memcmp(cert2->cache_info.identity_digest, cert1->cache_info.identity_digest,DIGEST_LEN)<0); - tt_assert(memcmp(cert1->cache_info.identity_digest, + tt_assert(fast_memcmp(cert1->cache_info.identity_digest, cert3->cache_info.identity_digest,DIGEST_LEN)<0); test_same_voter(smartlist_get(con->voters, 1), smartlist_get(v2->voters, 0));