diff options
| -rw-r--r-- | changes/assert_nonfatal | 5 | ||||
| -rw-r--r-- | src/common/include.am | 2 | ||||
| -rw-r--r-- | src/common/util.c | 17 | ||||
| -rw-r--r-- | src/common/util.h | 41 | ||||
| -rw-r--r-- | src/common/util_bug.c | 53 | ||||
| -rw-r--r-- | src/common/util_bug.h | 150 | ||||
| -rw-r--r-- | src/or/connection.c | 6 | ||||
| -rw-r--r-- | src/or/main.c | 8 | ||||
| -rw-r--r-- | src/or/policies.c | 2 | ||||
| -rw-r--r-- | src/or/rephist.c | 4 |
10 files changed, 219 insertions, 69 deletions
diff --git a/changes/assert_nonfatal b/changes/assert_nonfatal new file mode 100644 index 0000000000..0cbee4419b --- /dev/null +++ b/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. diff --git a/src/common/include.am b/src/common/include.am index 5afb30da6a..f978eeeaf6 100644 --- a/src/common/include.am +++ b/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 diff --git a/src/common/util.c b/src/common/util.c index 2351faf503..de6867e47a 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -105,23 +105,6 @@ #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 * ===== */ #ifdef USE_DMALLOC diff --git a/src/common/util.h b/src/common/util.h index ebcf88b32d..814c8622a2 100644 --- a/src/common/util.h +++ b/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; diff --git a/src/common/util_bug.c b/src/common/util_bug.c new file mode 100644 index 0000000000..e3e1d6df90 --- /dev/null +++ b/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); +} + diff --git a/src/common/util_bug.h b/src/common/util_bug.h new file mode 100644 index 0000000000..26134502c6 --- /dev/null +++ b/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 + diff --git a/src/or/connection.c b/src/or/connection.c index 78178f92fb..1bd1a92e39 100644 --- a/src/or/connection.c +++ b/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; } diff --git a/src/or/main.c b/src/or/main.c index bf5b2b823a..fba9799a60 100644 --- a/src/or/main.c +++ b/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. */ diff --git a/src/or/policies.c b/src/or/policies.c index f9718b6a95..2703d7edef 100644 --- a/src/or/policies.c +++ b/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)); } diff --git a/src/or/rephist.c b/src/or/rephist.c index fe0ca91c25..b94ad29650 100644 --- a/src/or/rephist.c +++ b/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); } |