Revise our assertion and bug macros to work with -Wparentheses

On GCC and Clang, there's a feature to warn you about bad
conditionals like "if (a = b)", which should be "if (a == b)".
However, they don't warn you if there are extra parentheses around
"a = b".

Unfortunately, the tor_assert() macro and all of its kin have been
passing their inputs through stuff like PREDICT_UNLIKELY(expr) or
PREDICT_UNLIKELY(!(expr)), both of which expand to stuff with more
parentheses around "expr", thus suppressing these warnings.

To fix this, this patch introduces new macros that do not wrap
expr.  They're only used when GCC or Clang is enabled (both define
__GNUC__), since they require GCC's "({statement expression})"
syntax extension.  They're only used when we're building the
unit-test variant of the object files, since they suppress the
branch-prediction hints.

I've confirmed that tor_assert(), tor_assert_nonfatal(),
tor_assert_nonfatal_once(), BUG(), and IF_BUG_ONCE() all now give
compiler warnings when their argument is an assignment expression.

Fixes bug 27709.

Bugfix on 0.0.6, where we first introduced the "tor_assert()" macro.

2 files changed, 44 insertions, 13 deletions

diff --git a/changes/bug27709 b/changes/bug27709
new file mode 100644
index 0000000000..49e87cbb0a
--- /dev/null
+++ b/changes/bug27709
@@ -0,0 +1,4 @@
+  o Minor bugfixes (code safety):
+    - Rewrite our assertion macros so that they no longer suppress
+      the compiler's -Wparentheses warnings on their inputs. Fixes bug 27709;
+      bugfix on 0.0.6.
diff --git a/src/common/util_bug.h b/src/common/util_bug.h
index 0695806911..22ec37d38f 100644
--- a/src/common/util_bug.h
+++ b/src/common/util_bug.h
@@ -29,6 +29,31 @@
 #error "Sorry; we don't support building with NDEBUG."
 #endif
 
+#if defined(TOR_UNIT_TESTS) && defined(__GNUC__)
+/* We define this GCC macro as a replacement for PREDICT_UNLIKELY() in this
+ * header, so that in our unit test builds, we'll get compiler warnings about
+ * stuff like tor_assert(n = 5).
+ *
+ * The key here is that (e) is wrapped in exactly one layer of parentheses,
+ * and then passed right to a conditional.  If you do anything else to the
+ * expression here, or introduce any more parentheses, the compiler won't
+ * help you.
+ */
+#define ASSERT_PREDICT_UNLIKELY_(e)             \
+  ({                                            \
+    int tor__assert_tmp_value__;                \
+    if (e)                                      \
+      tor__assert_tmp_value__ = 1;              \
+    else                                        \
+      tor__assert_tmp_value__ = 0;              \
+    tor__assert_tmp_value__;                    \
+  })
+#define ASSERT_PREDICT_LIKELY_(e) ASSERT_PREDICT_UNLIKELY_(e)
+#else
+#define ASSERT_PREDICT_UNLIKELY_(e) PREDICT_UNLIKELY(e)
+#define ASSERT_PREDICT_LIKELY_(e) PREDICT_LIKELY(e)
+#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. */
@@ -40,7 +65,8 @@
 /** 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))) {                                      \
+  if (ASSERT_PREDICT_LIKELY_(expr)) {                                   \
+  } else {                                                              \
     tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, #expr);     \
     abort();                                                            \
   } STMT_END
@@ -77,7 +103,7 @@
 #define tor_assert_nonfatal_unreached_once() tor_assert(0)
 #define tor_assert_nonfatal_once(cond) tor_assert((cond))
 #define BUG(cond)                                                       \
-  (PREDICT_UNLIKELY(cond) ?                                             \
+  (ASSERT_PREDICT_UNLIKELY_(cond) ?                                     \
    (tor_assertion_failed_(SHORT_FILE__,__LINE__,__func__,"!("#cond")"), \
     abort(), 1)                                                         \
    : 0)
@@ -86,14 +112,15 @@
 #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) (PREDICT_UNLIKELY(cond) ? 1 : 0)
+#define BUG(cond) (ASSERT_PREDICT_UNLIKELY_(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);  \
+  if (ASSERT_PREDICT_LIKELY_(cond)) {                                   \
+  } else {                                                              \
+    tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 0);      \
   }                                                                     \
   STMT_END
 #define tor_assert_nonfatal_unreached_once() STMT_BEGIN                 \
@@ -105,13 +132,14 @@
   STMT_END
 #define tor_assert_nonfatal_once(cond) STMT_BEGIN                       \
   static int warning_logged__ = 0;                                      \
-  if (!warning_logged__ && PREDICT_UNLIKELY(!(cond))) {                 \
+  if (ASSERT_PREDICT_LIKELY_(cond)) {                                   \
+  } else if (!warning_logged__) {                                       \
     warning_logged__ = 1;                                               \
     tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, #cond, 1);      \
   }                                                                     \
   STMT_END
 #define BUG(cond)                                                       \
-  (PREDICT_UNLIKELY(cond) ?                                             \
+  (ASSERT_PREDICT_UNLIKELY_(cond) ?                                     \
    (tor_bug_occurred_(SHORT_FILE__,__LINE__,__func__,"!("#cond")",0), 1) \
    : 0)
 #endif
@@ -120,17 +148,17 @@
 #define IF_BUG_ONCE__(cond,var)                                         \
   if (( {                                                               \
       static int var = 0;                                               \
-      int bool_result = (cond);                                         \
-      if (PREDICT_UNLIKELY(bool_result) && !var) {                      \
+      int bool_result = !!(cond);                                       \
+      if (bool_result && !var) {                                        \
         var = 1;                                                        \
         tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__,             \
                           "!("#cond")", 1);                             \
       }                                                                 \
-      PREDICT_UNLIKELY(bool_result); } ))
+      bool_result; } ))
 #else
 #define IF_BUG_ONCE__(cond,var)                                         \
   static int var = 0;                                                   \
-  if (PREDICT_UNLIKELY(cond) ?                                          \
+  if ((cond) ?                                                          \
       (var ? 1 :                                                        \
        (var=1,                                                          \
         tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__,             \
@@ -148,7 +176,7 @@
  */
 
 #define IF_BUG_ONCE(cond)                                       \
-  IF_BUG_ONCE__((cond),                                         \
+  IF_BUG_ONCE__(ASSERT_PREDICT_UNLIKELY_(cond),                 \
                 IF_BUG_ONCE_VARNAME__(__LINE__))
 
 /** Define this if you want Tor to crash when any problem comes up,
@@ -170,4 +198,3 @@ void tor_set_failed_assertion_callback(void (*fn)(void));
 #endif
 
 #endif
-