October 2018 - tor-commits - lists.torproject.org

[tor/release-0.3.3] Revise our assertion and bug macros to work with -Wparentheses
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit bb465be085ff8d1640f1d1c0bbb65605d85b5528 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Sep 14 11:39:37 2018 -0400 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(-) diff --git a/changes/bug27709 b/changes/bug27709 new file mode 100644 index 000000000..49e87cbb0 --- /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 069580691..22ec37d38 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 -

1 0

[tor/release-0.2.9] Merge branch 'maint-0.2.9' into release-0.2.9
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit 9a60e46ae98cd8e33091bc339b2c50a407806bc3 Merge: dcb6924cd b11339965 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:34:41 2018 -0400 Merge branch 'maint-0.2.9' into release-0.2.9 changes/bug27709 | 4 ++++ src/common/util_bug.h | 57 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 13 deletions(-)

1 0

[tor/master] Adjust tor_assert_unreached()
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit 796e36e535d1713fd1168772669199ea9189ba61 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:46:24 2018 -0400 Adjust tor_assert_unreached() This version of the macro makes it obvious to the compiler that "tor_assert_unreached()" will always abort(). --- src/common/util_bug.h | 7 ++++++- src/or/hs_common.c | 7 +++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/common/util_bug.h b/src/common/util_bug.h index c274355f3..be0001e84 100644 --- a/src/common/util_bug.h +++ b/src/common/util_bug.h @@ -102,7 +102,12 @@ } STMT_END #endif /* defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS) */ -#define tor_assert_unreached() tor_assert(0) +#define tor_assert_unreached() \ + STMT_BEGIN { \ + tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, \ + "line should be unreached"); \ + abort(); \ + } STMT_END /* 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 diff --git a/src/or/hs_common.c b/src/or/hs_common.c index 10b56c0ba..e586516c8 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -523,7 +523,7 @@ rend_data_get_address(const rend_data_t *rend_data) return TO_REND_DATA_V2(rend_data)->onion_address; default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -546,7 +546,7 @@ rend_data_get_desc_id(const rend_data_t *rend_data, uint8_t replica, return TO_REND_DATA_V2(rend_data)->descriptor_id[replica]; default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -569,7 +569,7 @@ rend_data_get_pk_digest(const rend_data_t *rend_data, size_t *len_out) } default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -1814,4 +1814,3 @@ hs_inc_rdv_stream_counter(origin_circuit_t *circ) tor_assert_nonfatal_unreached(); } } -

1 0

[tor/release-0.3.3] Merge branch 'bug27709_029' into maint-0.2.9
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit b113399658934f3a4526387a63429ec4377ca8be Merge: 717507e3b 5e582c7ff Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:34:39 2018 -0400 Merge branch 'bug27709_029' into maint-0.2.9 changes/bug27709 | 4 ++++ src/common/util_bug.h | 57 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 13 deletions(-)

1 0

[tor/release-0.3.3] Merge branch 'maint-0.2.9' into maint-0.3.3
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit dff7d3d00ad5e3f9e59cd4b11f302601790d1061 Merge: f7e93cf2e b11339965 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:37:49 2018 -0400 Merge branch 'maint-0.2.9' into maint-0.3.3 changes/bug27709 | 4 ++++ src/common/util_bug.h | 57 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 13 deletions(-) diff --cc src/common/util_bug.h index be549fde0,f3e5b7c96..c274355f3 --- a/src/common/util_bug.h +++ b/src/common/util_bug.h @@@ -53,8 -27,37 +53,37 @@@ * security-critical properties. */ #error "Sorry; we don't support building with NDEBUG." -#endif +#endif /* defined(NDEBUG) */ + #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. + * + * 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) \ + ({ \ + 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. */ @@@ -140,10 -143,10 +172,10 @@@ extern int bug_macro_deadcode_dummy__ } \ STMT_END #define BUG(cond) \ - (PREDICT_UNLIKELY(cond) ? \ + (ASSERT_PREDICT_UNLIKELY_(cond) ? \ (tor_bug_occurred_(SHORT_FILE__,__LINE__,__func__,"!("#cond")",0), 1) \ : 0) -#endif +#endif /* defined(ALL_BUGS_ARE_FATAL) || ... */ #ifdef __GNUC__ #define IF_BUG_ONCE__(cond,var) \ @@@ -155,11 -158,11 +187,11 @@@ tor_bug_occurred_(SHORT_FILE__, __LINE__, __func__, \ "!("#cond")", 1); \ } \ - PREDICT_UNLIKELY(bool_result); } )) + bool_result; } )) -#else +#else /* !(defined(__GNUC__)) */ #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__, \ @@@ -196,7 -199,6 +228,6 @@@ void tor_capture_bugs_(int n) void tor_end_capture_bugs_(void); const struct smartlist_t *tor_get_captured_bug_log_(void); void tor_set_failed_assertion_callback(void (*fn)(void)); -#endif +#endif /* defined(TOR_UNIT_TESTS) */ -#endif +#endif /* !defined(TOR_UTIL_BUG_H) */ -

1 0

[tor/release-0.3.3] Merge branch 'maint-0.3.3' into release-0.3.3
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit d7e0dc0056e5a82e298eb404f81c516f46657456 Merge: 49da1e9f6 796e36e53 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:48:35 2018 -0400 Merge branch 'maint-0.3.3' into release-0.3.3 src/common/util_bug.h | 7 ++++++- src/or/hs_common.c | 7 +++---- 2 files changed, 9 insertions(+), 5 deletions(-)

1 0

[tor/release-0.3.3] Adjust tor_assert_unreached()
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit 796e36e535d1713fd1168772669199ea9189ba61 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:46:24 2018 -0400 Adjust tor_assert_unreached() This version of the macro makes it obvious to the compiler that "tor_assert_unreached()" will always abort(). --- src/common/util_bug.h | 7 ++++++- src/or/hs_common.c | 7 +++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/common/util_bug.h b/src/common/util_bug.h index c274355f3..be0001e84 100644 --- a/src/common/util_bug.h +++ b/src/common/util_bug.h @@ -102,7 +102,12 @@ } STMT_END #endif /* defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS) */ -#define tor_assert_unreached() tor_assert(0) +#define tor_assert_unreached() \ + STMT_BEGIN { \ + tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, \ + "line should be unreached"); \ + abort(); \ + } STMT_END /* 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 diff --git a/src/or/hs_common.c b/src/or/hs_common.c index 10b56c0ba..e586516c8 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -523,7 +523,7 @@ rend_data_get_address(const rend_data_t *rend_data) return TO_REND_DATA_V2(rend_data)->onion_address; default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -546,7 +546,7 @@ rend_data_get_desc_id(const rend_data_t *rend_data, uint8_t replica, return TO_REND_DATA_V2(rend_data)->descriptor_id[replica]; default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -569,7 +569,7 @@ rend_data_get_pk_digest(const rend_data_t *rend_data, size_t *len_out) } default: /* We should always have a supported version. */ - tor_assert(0); + tor_assert_unreached(); } } @@ -1814,4 +1814,3 @@ hs_inc_rdv_stream_counter(origin_circuit_t *circ) tor_assert_nonfatal_unreached(); } } -

1 0

[tor/release-0.3.3] Merge branch 'maint-0.3.3' into release-0.3.3
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit 49da1e9f6ac8da2850ef2d2024539a5001bd9dc7 Merge: 8f872e945 dff7d3d00 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:37:52 2018 -0400 Merge branch 'maint-0.3.3' into release-0.3.3 changes/bug27709 | 4 ++++ src/common/util_bug.h | 57 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 13 deletions(-)

1 0

[tor/release-0.3.4] Merge branch 'maint-0.3.4' into release-0.3.4
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit 7e9f1d6992fd815c973049ed621daacf16ff0201 Merge: 294f5bb7e 3462f8ed6 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:48:35 2018 -0400 Merge branch 'maint-0.3.4' into release-0.3.4 src/common/util_bug.h | 7 ++++++- src/or/hs_common.c | 7 +++---- 2 files changed, 9 insertions(+), 5 deletions(-)

1 0

[tor/release-0.3.4] Merge branch 'bug27709_029' into maint-0.2.9
by nickm＠torproject.org 15 Oct '18

15 Oct '18

commit b113399658934f3a4526387a63429ec4377ca8be Merge: 717507e3b 5e582c7ff Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Oct 15 10:34:39 2018 -0400 Merge branch 'bug27709_029' into maint-0.2.9 changes/bug27709 | 4 ++++ src/common/util_bug.h | 57 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 13 deletions(-)

1 0