From f17591b8e504af9e088bd9df900639c83f5300fe Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Oct 2019 12:42:26 -0400 Subject: [PATCH 1/3] Rename max_in_sl to max_in_u16_sl, and expose it as STATIC. Since we want to make this function slightly more visible for testing purposes, it needs a better name. --- src/lib/dispatch/.may_include | 1 + src/lib/dispatch/dispatch_cfg.h | 6 ++++++ src/lib/dispatch/dispatch_new.c | 10 ++++++---- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/lib/dispatch/.may_include b/src/lib/dispatch/.may_include index 7f2df5859f..884f4c0dbc 100644 --- a/src/lib/dispatch/.may_include +++ b/src/lib/dispatch/.may_include @@ -8,3 +8,4 @@ lib/dispatch/*.h lib/intmath/*.h lib/log/*.h lib/malloc/*.h +lib/testsupport/*.h \ No newline at end of file diff --git a/src/lib/dispatch/dispatch_cfg.h b/src/lib/dispatch/dispatch_cfg.h index 61fade7240..348dce8d40 100644 --- a/src/lib/dispatch/dispatch_cfg.h +++ b/src/lib/dispatch/dispatch_cfg.h @@ -8,6 +8,7 @@ #define TOR_DISPATCH_CFG_H #include "lib/dispatch/msgtypes.h" +#include "lib/testsupport/testsupport.h" /** * A "dispatch_cfg" is the configuration used to set up a dispatcher. @@ -36,4 +37,9 @@ int dcfg_add_recv(dispatch_cfg_t *cfg, message_id_t msg, void dcfg_free_(dispatch_cfg_t *cfg); +#ifdef DISPATCH_NEW_PRIVATE +struct smartlist_t; +STATIC int max_in_u16_sl(const struct smartlist_t *sl, int dflt); +#endif + #endif /* !defined(TOR_DISPATCH_CFG_H) */ diff --git a/src/lib/dispatch/dispatch_new.c b/src/lib/dispatch/dispatch_new.c index b89ef43ea7..4467813064 100644 --- a/src/lib/dispatch/dispatch_new.c +++ b/src/lib/dispatch/dispatch_new.c @@ -9,6 +9,7 @@ * \brief Code to construct a dispatch_t from a dispatch_cfg_t. **/ +#define DISPATCH_NEW_PRIVATE #define DISPATCH_PRIVATE #include "orconfig.h" @@ -26,8 +27,8 @@ /** Given a smartlist full of (possibly NULL) pointers to uint16_t values, * return the largest value, or dflt if the list is empty. */ -static int -max_in_sl(const smartlist_t *sl, int dflt) +STATIC int +max_in_u16_sl(const smartlist_t *sl, int dflt) { uint16_t *maxptr = NULL; SMARTLIST_FOREACH_BEGIN(sl, uint16_t *, u) { @@ -118,11 +119,12 @@ dispatch_new(const dispatch_cfg_t *cfg) smartlist_len(cfg->recv_by_msg)) + 1; /* Any channel that any message has counts towards the number of channels. */ - const size_t n_chans = (size_t) MAX(1, max_in_sl(cfg->chan_by_msg,0)) + 1; + const size_t n_chans = (size_t) + MAX(1, max_in_u16_sl(cfg->chan_by_msg,0)) + 1; /* Any type that a message has, or that has functions, counts towards * the number of types. */ - const size_t n_types = (size_t) MAX(max_in_sl(cfg->type_by_msg,0), + const size_t n_types = (size_t) MAX(max_in_u16_sl(cfg->type_by_msg,0), smartlist_len(cfg->fns_by_type)) + 1; d->n_msgs = n_msgs; From 34bbdaf5d4673491216b64f5ab983518b3f8c7d1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Oct 2019 12:52:13 -0400 Subject: [PATCH 2/3] Add a test for max_u16_in_sl(). This test does not currently pass, because of bug 31898. --- src/test/test_dispatch.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/test/test_dispatch.c b/src/test/test_dispatch.c index d6fe7e781a..a62c18e0c9 100644 --- a/src/test/test_dispatch.c +++ b/src/test/test_dispatch.c @@ -1,6 +1,7 @@ /* Copyright (c) 2018, The Tor Project, Inc. */ /* See LICENSE for licensing information */ +#define DISPATCH_NEW_PRIVATE #define DISPATCH_PRIVATE #include "test/test.h" @@ -19,6 +20,33 @@ static dispatch_t *dispatcher_in_use=NULL; +static void +test_dispatch_max_in_u16_sl(void *arg) +{ + (void)arg; + smartlist_t *sl = smartlist_new(); + uint16_t nums[] = { 10, 20, 30 }; + tt_int_op(-1, OP_EQ, max_in_u16_sl(sl, -1)); + + smartlist_add(sl, NULL); + tt_int_op(-1, OP_EQ, max_in_u16_sl(sl, -1)); + + smartlist_add(sl, &nums[1]); + tt_int_op(20, OP_EQ, max_in_u16_sl(sl, -1)); + + smartlist_add(sl, &nums[0]); + tt_int_op(20, OP_EQ, max_in_u16_sl(sl, -1)); + + smartlist_add(sl, NULL); + tt_int_op(20, OP_EQ, max_in_u16_sl(sl, -1)); + + smartlist_add(sl, &nums[2]); + tt_int_op(30, OP_EQ, max_in_u16_sl(sl, -1)); + + done: + smartlist_free(sl); +} + /* Construct an empty dispatch_t. */ static void test_dispatch_empty(void *arg) @@ -240,6 +268,7 @@ test_dispatch_bad_type_setup(void *arg) { #name, test_dispatch_ ## name, TT_FORK, NULL, NULL } struct testcase_t dispatch_tests[] = { + T(max_in_u16_sl), T(empty), T(simple), T(no_recipient), From 2b825a1a2e6e79fa71b0e038241d2107aaf30d4b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Oct 2019 12:55:42 -0400 Subject: [PATCH 3/3] Fix a crash bug in max_u16_in_sl() The documentation for this function says that the smartlist can contain NULLs, but the code only handled NULLs if they were at the start of the list. We didn't notice this for a long time, because when Tor is run normally, the sequence of msg_id_t is densely packed, and so this list (mapping msg_id_t to channel_id_t) contains no NULL elements. We could only run into this bug: * when Tor was running in embedded mode, and starting more than once. * when Tor ran first with more pubsub messages enabled, and then later with fewer. * When the second run (the one with fewer enabled pubsub messages) had at least some messages enabled, and those messages were not the ones with numerically highest msg_id_t values. Fixes bug 31898; bugfix on 47de9c7b0a828de7fb8129413db70bc4e4ecac6d in 0.4.1.1-alpha. --- changes/bug31898 | 4 ++++ src/lib/dispatch/dispatch_new.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changes/bug31898 diff --git a/changes/bug31898 b/changes/bug31898 new file mode 100644 index 0000000000..6f3e0a5465 --- /dev/null +++ b/changes/bug31898 @@ -0,0 +1,4 @@ + o Major bugfixes (embedded Tor): + - Avoid a possible crash when restarting Tor in embedded mode and + enabling a different set of publish/subscribe messages. Fixes bug + 31898; bugfix on 0.4.1.1-alpha. diff --git a/src/lib/dispatch/dispatch_new.c b/src/lib/dispatch/dispatch_new.c index 4467813064..d8e59d610a 100644 --- a/src/lib/dispatch/dispatch_new.c +++ b/src/lib/dispatch/dispatch_new.c @@ -34,7 +34,7 @@ max_in_u16_sl(const smartlist_t *sl, int dflt) SMARTLIST_FOREACH_BEGIN(sl, uint16_t *, u) { if (!maxptr) maxptr = u; - else if (*u > *maxptr) + else if (u && *u > *maxptr) maxptr = u; } SMARTLIST_FOREACH_END(u);