Merge branch 'assert_nonfatal_squashed'

changes/assert_nonfatal

@@ -0,0 +1,5 @@
+  o Minor features (safety, debugging):
+
+    * Add a set of macros to check nonfatal assertions, for internal
+      use. Migrating more of our checks to these should help us avoid
+      needless crash bugs. Closes ticket 18613.

src/common/include.am

@@ -68,6 +68,7 @@ LIBOR_A_SOURCES = \
   src/common/log.c					\
   src/common/memarea.c					\
   src/common/util.c					\
+  src/common/util_bug.c					\
   src/common/util_format.c				\
   src/common/util_process.c				\
   src/common/sandbox.c					\
@@ -139,6 +140,7 @@ COMMONHEADERS = \
   src/common/torlog.h				\
   src/common/tortls.h				\
   src/common/util.h				\
+  src/common/util_bug.h				\
   src/common/util_format.h			\
   src/common/util_process.h			\
   src/common/workqueue.h

src/common/util.c

@@ -104,23 +104,6 @@
 #undef MALLOC_ZERO_WORKS
 #endif
 
-/* =====
- * Assertion helper.
- * ===== */
-/** Helper for tor_assert: report the assertion failure. */
-void
-tor_assertion_failed_(const char *fname, unsigned int line,
-                      const char *func, const char *expr)
-{
-  char buf[256];
-  log_err(LD_BUG, "%s:%u: %s: Assertion %s failed; aborting.",
-          fname, line, func, expr);
-  tor_snprintf(buf, sizeof(buf),
-               "Assertion %s failed in %s at %s:%u",
-               expr, func, fname, line);
-  log_backtrace(LOG_ERR, LD_BUG, buf);
-}
-
 /* =====
  * Memory management
  * ===== */

src/common/util.h

@@ -22,6 +22,7 @@
 /* for the correct alias to struct stat */
 #include <sys/stat.h>
 #endif
+#include "util_bug.h"
 
 #ifndef O_BINARY
 #define O_BINARY 0
@@ -33,41 +34,6 @@
 #define O_NOFOLLOW 0
 #endif
 
-/* Replace assert() with a variant that sends failures to the log before
- * calling assert() normally.
- */
-#ifdef NDEBUG
-/* Nobody should ever want to build with NDEBUG set.  99% of our asserts will
- * be outside the critical path anyway, so it's silly to disable bug-checking
- * throughout the entire program just because a few asserts are slowing you
- * down.  Profile, optimize the critical path, and keep debugging on.
- *
- * And I'm not just saying that because some of our asserts check
- * security-critical properties.
- */
-#error "Sorry; we don't support building with NDEBUG."
-#endif
-
-/* Sometimes we don't want to use assertions during branch coverage tests; it
- * leads to tons of unreached branches which in reality are only assertions we
- * didn't hit. */
-#if defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS)
-#define tor_assert(a) STMT_BEGIN                                        \
-  (void)(a);                                                            \
-  STMT_END
-#else
-/** Like assert(3), but send assertion failures to the log as well as to
- * stderr. */
-#define tor_assert(expr) STMT_BEGIN                                     \
-  if (PREDICT_UNLIKELY(!(expr))) {                                      \
-    tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, #expr);     \
-    abort();                                                            \
-  } STMT_END
-#endif
-
-void tor_assertion_failed_(const char *fname, unsigned int line,
-                           const char *func, const char *expr);
-
 /* If we're building with dmalloc, we want all of our memory allocation
  * functions to take an extra file/line pair of arguments.  If not, not.
  * We define DMALLOC_PARAMS to the extra parameters to insert in the
@@ -81,11 +47,6 @@ void tor_assertion_failed_(const char *fname, unsigned int line,
 #define DMALLOC_ARGS
 #endif
 
-/** Define this if you want Tor to crash when any problem comes up,
- * so you can get a coredump and track things down. */
-// #define tor_fragile_assert() tor_assert(0)
-#define tor_fragile_assert()
-
 /* Memory management */
 void *tor_malloc_(size_t size DMALLOC_PARAMS) ATTR_MALLOC;
 void *tor_malloc_zero_(size_t size DMALLOC_PARAMS) ATTR_MALLOC;

src/common/util_bug.c

@@ -0,0 +1,53 @@
+/* Copyright (c) 2003, Roger Dingledine
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2016, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file util_bug.c
+ **/
+
+#include "orconfig.h"
+#include "util_bug.h"
+#include "torlog.h"
+#include "backtrace.h"
+
+/** Helper for tor_assert: report the assertion failure. */
+void
+tor_assertion_failed_(const char *fname, unsigned int line,
+                      const char *func, const char *expr)
+{
+  char buf[256];
+  log_err(LD_BUG, "%s:%u: %s: Assertion %s failed; aborting.",
+          fname, line, func, expr);
+  tor_snprintf(buf, sizeof(buf),
+               "Assertion %s failed in %s at %s:%u",
+               expr, func, fname, line);
+  log_backtrace(LOG_ERR, LD_BUG, buf);
+}
+
+/** Helper for tor_assert_nonfatal: report the assertion failure. */
+void
+tor_bug_occurred_(const char *fname, unsigned int line,
+                  const char *func, const char *expr,
+                  int once)
+{
+  char buf[256];
+  const char *once_str = once ?
+    " (Future instances of this warning will be silenced.)": "";
+  if (! expr) {
+    log_warn(LD_BUG, "%s:%u: %s: This line should not have been reached.%s",
+             fname, line, func, once_str);
+    tor_snprintf(buf, sizeof(buf),
+                 "Line unexpectedly reached at %s at %s:%u",
+                 func, fname, line);
+  } else {
+    log_warn(LD_BUG, "%s:%u: %s: Non-fatal assertion %s failed.%s",
+             fname, line, func, expr, once_str);
+    tor_snprintf(buf, sizeof(buf),
+                 "Non-fatal assertion %s failed in %s at %s:%u",
+                 expr, func, fname, line);
+  }
+  log_backtrace(LOG_WARN, LD_BUG, buf);
+}
+

src/common/util_bug.h

@@ -0,0 +1,150 @@
+/* Copyright (c) 2003-2004, Roger Dingledine
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2016, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file util_bug.h
+ **/
+
+#ifndef TOR_UTIL_BUG_H
+#define TOR_UTIL_BUG_H
+
+#include "orconfig.h"
+#include "compat.h"
+#include "testsupport.h"
+
+/* Replace assert() with a variant that sends failures to the log before
+ * calling assert() normally.
+ */
+#ifdef NDEBUG
+/* Nobody should ever want to build with NDEBUG set.  99% of our asserts will
+ * be outside the critical path anyway, so it's silly to disable bug-checking
+ * throughout the entire program just because a few asserts are slowing you
+ * down.  Profile, optimize the critical path, and keep debugging on.
+ *
+ * And I'm not just saying that because some of our asserts check
+ * security-critical properties.
+ */
+#error "Sorry; we don't support building with NDEBUG."
+#endif
+
+/* Sometimes we don't want to use assertions during branch coverage tests; it
+ * leads to tons of unreached branches which in reality are only assertions we
+ * didn't hit. */
+#if defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS)
+#define tor_assert(a) STMT_BEGIN                                        \
+  (void)(a);                                                            \
+  STMT_END
+#else
+/** Like assert(3), but send assertion failures to the log as well as to
+ * stderr. */
+#define tor_assert(expr) STMT_BEGIN                                     \
+  if (PREDICT_UNLIKELY(!(expr))) {                                      \
+    tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, #expr);     \
+    abort();                                                            \
+  } STMT_END
+#endif
+
+#define tor_assert_unreached() tor_assert(0)
+
+/* Non-fatal bug assertions. The "unreached" variants mean "this line should
+ * never be reached." The "once" variants mean "Don't log a warning more than
+ * once".
+ *
+ * The 'BUG' macro checks a boolean condition and logs an error message if it
+ * is true.  Example usage:
+ *   if (BUG(x == NULL))
+ *     return -1;
+ */
+
+#ifdef ALL_BUGS_ARE_FATAL
+#define tor_assert_nonfatal_unreached() tor_assert(0)
+#define tor_assert_nonfatal(cond) tor_assert((cond))
+#define tor_assert_nonfatal_unreached_once() tor_assert(0)
+#define tor_assert_nonfatal_once(cond) tor_assert((cond))
+#define BUG(cond)                                                       \
+  ((cond) ?                                                             \
+   (tor_assertion_failed_(SHORT_FILE__,__LINE__,__func__,#cond), abort(), 1) \
+   : 0)
+#elif defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS)
+#define tor_assert_nonfatal_unreached() STMT_NIL
+#define tor_assert_nonfatal(cond) ((void)(cond))
+#define tor_assert_nonfatal_unreached_once() STMT_NIL
+#define tor_assert_nonfatal_once(cond) ((void)(cond))
+#define BUG(cond) ((cond) ? 1 : 0)
+#else /* Normal case, !ALL_BUGS_ARE_FATAL, !DISABLE_ASSERTS_IN_UNIT_TESTS */
+#define tor_assert_nonfatal_unreached() STMT_BEGIN                      \
+  tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, NULL, 0);         \
+  STMT_END
+#define tor_assert_nonfatal(cond) STMT_BEGIN                            \
+  if (PREDICT_UNLIKELY(!(cond))) {                                      \
+    tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 0);  \
+  }                                                                     \
+  STMT_END
+#define tor_assert_nonfatal_unreached_once() STMT_BEGIN                 \
+  static int warning_logged__ = 0;                                      \
+  if (!warning_logged__) {                                              \
+    warning_logged__ = 1;                                               \
+    tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, NULL, 1);       \
+  }                                                                     \
+  STMT_END
+#define tor_assert_nonfatal_once(cond) STMT_BEGIN                       \
+  static int warning_logged__ = 0;                                      \
+  if (!warning_logged__ && PREDICT_UNLIKELY(!(cond))) {                 \
+    warning_logged__ = 1;                                               \
+    tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 1);      \
+  }                                                                     \
+  STMT_END
+#define BUG(cond)                                                       \
+  ((cond) ?                                                             \
+   (tor_bug_occurred_(SHORT_FILE__,__LINE__,__func__,#cond,0), 1)       \
+   : 0)
+#endif
+
+#ifdef __GNUC__
+#define IF_BUG_ONCE__(cond,var)                                         \
+  if (({                                                                \
+      static int var = 0;                                               \
+      int bool_result = (cond);                                         \
+      if (bool_result && !var) {                                        \
+        var = 1;                                                        \
+        tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 1);  \
+      }                                                                 \
+      var; }))
+#else
+#define IF_BUG_ONCE__(cond,var)                                         \
+  static int var = 0;                                                   \
+  if ((cond) ?                                                          \
+      (var ? 1 :                                                        \
+       (var=1,                                                          \
+        tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 1),  \
+        1))                                                             \
+      : 0)
+#endif
+#define IF_BUG_ONCE_VARNAME_(a)               \
+  warning_logged_on_ ## a ## __
+#define IF_BUG_ONCE_VARNAME__(a)              \
+  IF_BUG_ONCE_VARNAME_(a)
+
+/** This macro behaves as 'if (bug(x))', except that it only logs its
+ * warning once, no matter how many times it triggers.
+ */
+
+#define IF_BUG_ONCE(cond)                                       \
+  IF_BUG_ONCE__((cond),                                         \
+                IF_BUG_ONCE_VARNAME__(__LINE__))
+
+/** Define this if you want Tor to crash when any problem comes up,
+ * so you can get a coredump and track things down. */
+// #define tor_fragile_assert() tor_assert_unreached(0)
+#define tor_fragile_assert() tor_assert_nonfatal_unreached_once()
+
+void tor_assertion_failed_(const char *fname, unsigned int line,
+                           const char *func, const char *expr);
+void tor_bug_occurred_(const char *fname, unsigned int line,
+                       const char *func, const char *expr,
+                       int once);
+
+#endif
+

src/or/connection.c

@@ -665,9 +665,7 @@ connection_free,(connection_t *conn))
     return;
   tor_assert(!connection_is_on_closeable_list(conn));
   tor_assert(!connection_in_array(conn));
-  if (conn->linked_conn) {
-    log_err(LD_BUG, "Called with conn->linked_conn still set.");
-    tor_fragile_assert();
+  if (BUG(conn->linked_conn)) {
     conn->linked_conn->linked_conn = NULL;
     if (! conn->linked_conn->marked_for_close &&
         conn->linked_conn->reading_from_linked_conn)
@@ -3644,7 +3642,7 @@ connection_read_to_buf(connection_t *conn, ssize_t *max_to_read,
        * take us over our read allotment, but really we shouldn't be
        * believing that SSL bytes are the same as TCP bytes anyway. */
       int r2 = read_to_buf_tls(or_conn->tls, pending, conn->inbuf);
-      if (r2<0) {
+      if (BUG(r2<0)) {
         log_warn(LD_BUG, "apparently, reading pending bytes can fail.");
         return -1;
       }

src/or/main.c

@@ -1643,8 +1643,8 @@ rotate_x509_certificate_callback(time_t now, const or_options_t *options)
    * TLS context. */
   log_info(LD_GENERAL,"Rotating tls context.");
   if (router_initialize_tls_context() < 0) {
-    log_warn(LD_BUG, "Error reinitializing TLS context");
-    tor_assert(0);
+    log_err(LD_BUG, "Error reinitializing TLS context");
+    tor_assert_unreached();
   }
 
   /* We also make sure to rotate the TLS connections themselves if they've
@@ -2563,9 +2563,7 @@ run_main_loop_once(void)
         return -1;
 #endif
     } else {
-      if (ERRNO_IS_EINPROGRESS(e))
-        log_warn(LD_BUG,
-                 "libevent call returned EINPROGRESS? Please report.");
+      tor_assert_nonfatal_once(! ERRNO_IS_EINPROGRESS(e));
       log_debug(LD_NET,"libevent call interrupted.");
       /* You can't trust the results of this poll(). Go back to the
        * top of the big for loop. */

src/or/policies.c

@@ -103,7 +103,7 @@ policy_expand_private(smartlist_t **policy)
        if (tor_addr_parse_mask_ports(private_nets[i], 0,
                                &newpolicy.addr,
                                &newpolicy.maskbits, &port_min, &port_max)<0) {
-         tor_assert(0);
+         tor_assert_unreached();
        }
        smartlist_add(tmp, addr_policy_get_canonical_entry(&newpolicy));
      }

src/or/rephist.c

@@ -3214,7 +3214,7 @@ rep_hist_free_all(void)
   rep_hist_desc_stats_term();
   total_descriptor_downloads = 0;
 
-  tor_assert(rephist_total_alloc == 0);
-  tor_assert(rephist_total_num == 0);
+  tor_assert_nonfatal(rephist_total_alloc == 0);
+  tor_assert_nonfatal_once(rephist_total_num == 0);
 }