commit 47de9c7b0a828de7fb8129413db70bc4e4ecac6d Author: Nick Mathewson nickm@torproject.org Date: Thu Mar 14 15:15:03 2019 -0400
Use actual pointers in dispatch_cfg.c.
Previously, I had used integers encoded as pointers. This introduced a flaw: NULL represented both the integer zero, and the absence of a setting. This in turn made the checks in cfg_msg_set_{type,chan}() not actually check for an altered value if the previous value had been set to zero.
Also, I had previously kept a pointer to a dispatch_fypefns_t rather than making a copy of it. This meant that if the dispatch_typefns_t were changed between defining the typefns and creating the dispatcher, we'd get the modified version.
Found while investigating coverage in pubsub_add_{pub,sub}_() --- src/lib/dispatch/dispatch_cfg.c | 23 ++++++++++++--------- src/lib/dispatch/dispatch_new.c | 46 ++++++++++++++++++++++------------------- 2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/src/lib/dispatch/dispatch_cfg.c b/src/lib/dispatch/dispatch_cfg.c index 26e37f469..b3a72ec22 100644 --- a/src/lib/dispatch/dispatch_cfg.c +++ b/src/lib/dispatch/dispatch_cfg.c @@ -37,9 +37,6 @@ dcfg_new(void) return cfg; }
-/** DOCDOC */ -#define ID_TO_VOID(id) ((void*)(uintptr_t)(id)) - /** * Associate a message with a datatype. Return 0 on success, -1 if a * different type was previously associated with the message ID. @@ -49,11 +46,12 @@ dcfg_msg_set_type(dispatch_cfg_t *cfg, message_id_t msg, msg_type_id_t type) { smartlist_grow(cfg->type_by_msg, msg+1); - void *oldval = smartlist_get(cfg->type_by_msg, msg); - if (oldval != NULL && oldval != ID_TO_VOID(type)) { + msg_type_id_t *oldval = smartlist_get(cfg->type_by_msg, msg); + if (oldval != NULL && *oldval != type) { return -1; } - smartlist_set(cfg->type_by_msg, msg, ID_TO_VOID(type)); + if (!oldval) + smartlist_set(cfg->type_by_msg, msg, tor_memdup(&type, sizeof(type))); return 0; }
@@ -66,11 +64,12 @@ dcfg_msg_set_chan(dispatch_cfg_t *cfg, message_id_t msg, channel_id_t chan) { smartlist_grow(cfg->chan_by_msg, msg+1); - void *oldval = smartlist_get(cfg->chan_by_msg, msg); - if (oldval != NULL && oldval != ID_TO_VOID(chan)) { + channel_id_t *oldval = smartlist_get(cfg->chan_by_msg, msg); + if (oldval != NULL && *oldval != chan) { return -1; } - smartlist_set(cfg->chan_by_msg, msg, ID_TO_VOID(chan)); + if (!oldval) + smartlist_set(cfg->chan_by_msg, msg, tor_memdup(&chan, sizeof(chan))); return 0; }
@@ -87,7 +86,8 @@ dcfg_type_set_fns(dispatch_cfg_t *cfg, msg_type_id_t type, if (oldfns && (oldfns->free_fn != fns->free_fn || oldfns->fmt_fn != fns->fmt_fn)) return -1; - smartlist_set(cfg->fns_by_type, type, (dispatch_typefns_t *) fns); + if (!oldfns) + smartlist_set(cfg->fns_by_type, type, tor_memdup(fns, sizeof(*fns))); return 0; }
@@ -123,6 +123,9 @@ dcfg_free_(dispatch_cfg_t *cfg) if (!cfg) return;
+ SMARTLIST_FOREACH(cfg->type_by_msg, msg_type_id_t *, id, tor_free(id)); + SMARTLIST_FOREACH(cfg->chan_by_msg, channel_id_t *, id, tor_free(id)); + SMARTLIST_FOREACH(cfg->fns_by_type, dispatch_typefns_t *, f, tor_free(f)); smartlist_free(cfg->type_by_msg); smartlist_free(cfg->chan_by_msg); smartlist_free(cfg->fns_by_type); diff --git a/src/lib/dispatch/dispatch_new.c b/src/lib/dispatch/dispatch_new.c index a2879016a..b89ef43ea 100644 --- a/src/lib/dispatch/dispatch_new.c +++ b/src/lib/dispatch/dispatch_new.c @@ -17,32 +17,36 @@ #include "lib/dispatch/dispatch_cfg.h" #include "lib/dispatch/dispatch_cfg_st.h"
+#include "lib/cc/ctassert.h" #include "lib/intmath/cmp.h" #include "lib/malloc/malloc.h" #include "lib/log/util_bug.h"
#include <string.h>
-/** Convert a void* in a smartlist to the corresponding integer. */ -#define VOID_TO_ID(p) ((intptr_t)(p)) - -/** Given a smartlist full of void* fields encoding intptr_t values, - * return the largest intptr_t, or dflt if the list is empty. */ -static intptr_t -max_in_sl(const smartlist_t *sl, intptr_t dflt) +/** 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) { - if (!smartlist_len(sl)) - return dflt; - void *as_ptr = smartlist_get(sl, 0); - intptr_t max = VOID_TO_ID(as_ptr); - SMARTLIST_FOREACH_BEGIN(sl, void *, p) { - intptr_t i = VOID_TO_ID(p); - if (i > max) - max = i; - } SMARTLIST_FOREACH_END(p); - return max; + uint16_t *maxptr = NULL; + SMARTLIST_FOREACH_BEGIN(sl, uint16_t *, u) { + if (!maxptr) + maxptr = u; + else if (*u > *maxptr) + maxptr = u; + } SMARTLIST_FOREACH_END(u); + + return maxptr ? *maxptr : dflt; }
+/* The above function is only safe to call if we are sure that channel_id_t + * and msg_type_id_t are really uint16_t. They should be so defined in + * msgtypes.h, but let's be extra cautious. + */ +CTASSERT(sizeof(uint16_t) == sizeof(msg_type_id_t)); +CTASSERT(sizeof(uint16_t) == sizeof(channel_id_t)); + /** Helper: Format an unformattable message auxiliary data item: just return a * copy of the string <>. */ static char * @@ -156,14 +160,14 @@ dispatch_new(const dispatch_cfg_t *cfg)
/* Fill in the empty entries in the dispatch tables: * types and channels for each message. */ - SMARTLIST_FOREACH_BEGIN(cfg->type_by_msg, smartlist_t *, type) { + SMARTLIST_FOREACH_BEGIN(cfg->type_by_msg, msg_type_id_t *, type) { if (d->table[type_sl_idx]) - d->table[type_sl_idx]->type = VOID_TO_ID(type); + d->table[type_sl_idx]->type = *type; } SMARTLIST_FOREACH_END(type);
- SMARTLIST_FOREACH_BEGIN(cfg->chan_by_msg, smartlist_t *, chan) { + SMARTLIST_FOREACH_BEGIN(cfg->chan_by_msg, channel_id_t *, chan) { if (d->table[chan_sl_idx]) - d->table[chan_sl_idx]->channel = VOID_TO_ID(chan); + d->table[chan_sl_idx]->channel = *chan; } SMARTLIST_FOREACH_END(chan);
return d;