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.

16 files changed, 69 insertions, 39 deletions

diff --git a/scripts/maint/checkSpace.pl b/scripts/maint/checkSpace.pl
index 906281112d..e90f5b330b 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 2841281927..81e04e94eb 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 4614ef94d5..5385bd871c 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 92ad3f5ec7..71495b660a 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 71b67906b7..56adc77f84 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 c7dd2a8af7..211ed7f8d2 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 7a6203aeea..57605ccfd1 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 87fa721089..6a78b21988 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 78e1b71014..a711b56d44 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 a62f80ef91..9c2411ede8 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 19564f5924..e672a416be 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 f1cf715f30..f373019b95 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 c28d5054a2..2d40283fb1 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 dcecb0b7dc..c8a9e6d384 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 542512bd44..9cae1e8dd8 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 5d9ae3c8a1..bfd89a917a 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));