From bb465be085ff8d1640f1d1c0bbb65605d85b5528 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Sep 2018 11:39:37 -0400 Subject: [PATCH 1/2] 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. --- changes/bug27709 | 4 ++++ src/common/util_bug.h | 53 ++++++++++++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 changes/bug27709 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 - From 5e582c7ffafd9075373131e5e55b69507f54206e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 15 Oct 2018 10:16:34 -0400 Subject: [PATCH 2/2] Explain a bit more about branch prediction in the unit-test case --- src/common/util_bug.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/common/util_bug.h b/src/common/util_bug.h index 22ec37d38f..f3e5b7c963 100644 --- a/src/common/util_bug.h +++ b/src/common/util_bug.h @@ -38,6 +38,10 @@ * 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. + * + * We only do this for the unit-test build case because it interferes with + * the likely-branch labeling. Note below that in the other case, we define + * these macros to just be synonyms for PREDICT_(UN)LIKELY. */ #define ASSERT_PREDICT_UNLIKELY_(e) \ ({ \