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}_()

2 files changed, 38 insertions, 31 deletions

diff --git a/src/lib/dispatch/dispatch_cfg.c b/src/lib/dispatch/dispatch_cfg.c
index 26e37f4694..b3a72ec22f 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 a2879016a7..b89ef43ea7 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;