From 4c88ebcf4728084cad0a1f177eb9cdb107388581 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 26 Sep 2019 13:31:32 +1000 Subject: [PATCH 1/3] log: Remove duplicate code and an outdated comment Preparation for 31854. --- src/lib/log/log.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/lib/log/log.c b/src/lib/log/log.c index d2002f6eae..994fdbdd9e 100644 --- a/src/lib/log/log.c +++ b/src/lib/log/log.c @@ -584,8 +584,7 @@ logv,(int severity, log_domain_mask_t domain, const char *funcname, /* check that severity is sane. Overrunning the masks array leads to * interesting and hard to diagnose effects */ raw_assert(severity >= LOG_ERR && severity <= LOG_DEBUG); - /* check that we've initialised the log mutex before we try to lock it */ - raw_assert(log_mutex_initialized); + LOCK_LOGS(); if ((! (domain & LD_NOCB)) && pending_cb_messages @@ -866,9 +865,6 @@ logs_close_sigsafe(void) * logfiles (it is probably present, but it might not be due to thread * racing issues). After this function is called, the caller shouldn't * refer to victim anymore. - * - * Long-term, we need to do something about races in the log subsystem - * in general. See bug 222 for more details. */ static void delete_log(logfile_t *victim) From 25c5322dfe228178391b37fffd3333abdf57baa5 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 26 Sep 2019 13:33:17 +1000 Subject: [PATCH 2/3] log: Define count, boundary, and all macros for domains and flags And do static checks on those macro definitions. Part of 31854. --- src/lib/log/log.c | 4 +++- src/lib/log/log.h | 15 +++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/lib/log/log.c b/src/lib/log/log.c index 994fdbdd9e..db4f154418 100644 --- a/src/lib/log/log.c +++ b/src/lib/log/log.c @@ -1337,7 +1337,9 @@ static const char *domain_list[] = { CTASSERT(ARRAY_LENGTH(domain_list) == N_LOGGING_DOMAINS + 1); -CTASSERT((UINT64_C(1)<<(N_LOGGING_DOMAINS-1)) < LOWEST_RESERVED_LD_FLAG_); +CTASSERT(HIGHEST_RESERVED_LD_DOMAIN_ < LD_ALL_DOMAINS); +CTASSERT(LD_ALL_DOMAINS < LOWEST_RESERVED_LD_FLAG_); +CTASSERT(LOWEST_RESERVED_LD_FLAG_ < LD_ALL_FLAGS); /** Return a bitmask for the log domain for which domain is the name, * or 0 if there is no such name. */ diff --git a/src/lib/log/log.h b/src/lib/log/log.h index da4bcbe608..b957fd1567 100644 --- a/src/lib/log/log.h +++ b/src/lib/log/log.h @@ -117,10 +117,21 @@ #define LD_BTRACK (UINT64_C(1)<<28) /** Message-passing backend. */ #define LD_MESG (UINT64_C(1)<<29) -#define N_LOGGING_DOMAINS 30 +/** The number of log domains. */ +#define N_LOGGING_DOMAINS 30 +/** The highest log domain */ +#define HIGHEST_RESERVED_LD_DOMAIN_ (UINT64_C(1)<<(N_LOGGING_DOMAINS - 1)) +/** All log domains. */ +#define LD_ALL_DOMAINS ((~(UINT64_C(0)))>>(64 - N_LOGGING_DOMAINS)) + +/** The number of log flags. */ +#define N_LOGGING_FLAGS 3 /** First bit that is reserved in log_domain_mask_t for non-domain flags. */ -#define LOWEST_RESERVED_LD_FLAG_ (UINT64_C(1)<<61) +#define LOWEST_RESERVED_LD_FLAG_ (UINT64_C(1)<<(64 - N_LOGGING_FLAGS)) +/** All log flags. */ +#define LD_ALL_FLAGS ((~(UINT64_C(0)))<<(64 - N_LOGGING_FLAGS)) + #ifdef TOR_UNIT_TESTS /** This log message should not be intercepted by mock_saving_logv */ #define LD_NO_MOCK (UINT64_C(1)<<61) From db329522ef6f0d0971111cbd07f35d54e0c7eced Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 26 Sep 2019 13:37:06 +1000 Subject: [PATCH 3/3] log: When initialising log domain masks, only set known log domains And add a runtime test that checks for unknown domains and flags. Fixes bug 31854; bugfix on 0.2.1.1-alpha. --- changes/bug31854 | 3 +++ src/lib/log/log.c | 12 ++++++++---- src/test/test_options.c | 6 +++--- 3 files changed, 14 insertions(+), 7 deletions(-) create mode 100644 changes/bug31854 diff --git a/changes/bug31854 b/changes/bug31854 new file mode 100644 index 0000000000..692a192fd9 --- /dev/null +++ b/changes/bug31854 @@ -0,0 +1,3 @@ + o Minor bugfixes (logging): + - When initialising log domain masks, only set known log domains. + Fixes bug 31854; bugfix on 0.2.1.1-alpha. diff --git a/src/lib/log/log.c b/src/lib/log/log.c index db4f154418..7c18bea0d9 100644 --- a/src/lib/log/log.c +++ b/src/lib/log/log.c @@ -628,6 +628,10 @@ void tor_log(int severity, log_domain_mask_t domain, const char *format, ...) { va_list ap; + + /* check that domain is composed of known domains and flags */ + raw_assert((domain & (LD_ALL_DOMAINS|LD_ALL_FLAGS)) == domain); + if (severity > log_global_min_severity_) return; va_start(ap,format); @@ -927,7 +931,7 @@ set_log_severity_config(int loglevelMin, int loglevelMax, raw_assert(loglevelMax >= LOG_ERR && loglevelMax <= LOG_DEBUG); memset(severity_out, 0, sizeof(log_severity_list_t)); for (i = loglevelMin; i >= loglevelMax; --i) { - severity_out->masks[SEVERITY_MASK_IDX(i)] = ~0u; + severity_out->masks[SEVERITY_MASK_IDX(i)] = LD_ALL_DOMAINS; } } @@ -1421,7 +1425,7 @@ parse_log_severity_config(const char **cfg_ptr, const char *dash, *space; char *sev_lo, *sev_hi; int low, high, i; - log_domain_mask_t domains = ~0u; + log_domain_mask_t domains = LD_ALL_DOMAINS; if (*cfg == '[') { int err = 0; @@ -1439,7 +1443,7 @@ parse_log_severity_config(const char **cfg_ptr, tor_free(domains_str); SMARTLIST_FOREACH_BEGIN(domains_list, const char *, domain) { if (!strcmp(domain, "*")) { - domains = ~0u; + domains = LD_ALL_DOMAINS; } else { log_domain_mask_t d; int negate=0; @@ -1535,7 +1539,7 @@ switch_logs_debug(void) LOCK_LOGS(); for (lf = logfiles; lf; lf=lf->next) { for (i = LOG_DEBUG; i >= LOG_ERR; --i) - lf->severities->masks[SEVERITY_MASK_IDX(i)] = ~0u; + lf->severities->masks[SEVERITY_MASK_IDX(i)] = LD_ALL_DOMAINS; } log_global_min_severity_ = get_min_log_level(); UNLOCK_LOGS(); diff --git a/src/test/test_options.c b/src/test/test_options.c index b3654ede7d..9eb5a43924 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -54,9 +54,9 @@ setup_log_callback(void) { log_severity_list_t lst; memset(&lst, 0, sizeof(lst)); - lst.masks[SEVERITY_MASK_IDX(LOG_ERR)] = ~0; - lst.masks[SEVERITY_MASK_IDX(LOG_WARN)] = ~0; - lst.masks[SEVERITY_MASK_IDX(LOG_NOTICE)] = ~0; + lst.masks[SEVERITY_MASK_IDX(LOG_ERR)] = LD_ALL_DOMAINS; + lst.masks[SEVERITY_MASK_IDX(LOG_WARN)] = LD_ALL_DOMAINS; + lst.masks[SEVERITY_MASK_IDX(LOG_NOTICE)] = LD_ALL_DOMAINS; add_callback_log(&lst, log_cback); mark_logs_temp(); }