tal: remove undefined behavior.

In particular, we were casting between struct prop_hdr ** and char **:
fixing this properly was quite a rework.

And in one case we were using a pre-realloc pointer in tests: though to be
fair, we tested it for equality.  But ubsan got upset anyway.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/ccan/tal/tal.c b/ccan/tal/tal.c
index 31e8e2f66f691aa3e80f360b8d12c0cbb52fc377..1230d8cacafcd2bfb05b659435480ba0ac77f9c5 100644 (file)
--- a/ccan/tal/tal.c
+++ b/ccan/tal/tal.c
@@ -28,7 +28,8 @@ enum prop_type {
 
 struct tal_hdr {
        struct list_node list;
-       struct prop_hdr *prop;
+       /* Use is_prop_hdr tell if this is a struct prop_hdr or string! */
+       char *prop;
        /* XOR with TAL_PTR_OBFUSTICATOR */
        intptr_t parent_child;
        size_t bytelen;
@@ -36,7 +37,8 @@ struct tal_hdr {
 
 struct prop_hdr {
        enum prop_type type;
-       struct prop_hdr *next;
+       /* Use is_prop_hdr to tell if this is a struct prop_hdr or string! */
+       char *next;
 };
 
 struct children {
@@ -72,7 +74,7 @@ static struct {
        struct tal_hdr hdr;
        struct children c;
 } null_parent = { { { &null_parent.hdr.list, &null_parent.hdr.list },
-                   &null_parent.c.hdr, TAL_PTR_OBFUSTICATOR, 0 },
+               (char *)&null_parent.c.hdr, TAL_PTR_OBFUSTICATOR, 0 },
                  { { CHILDREN, NULL },
                    &null_parent.hdr,
                    { { &null_parent.c.children.n,
@@ -123,9 +125,11 @@ void tal_cleanup(void)
 }
 
 /* We carefully start all real properties with a zero byte. */
-static bool is_literal(const struct prop_hdr *prop)
+static struct prop_hdr *is_prop_hdr(const char *ptr)
 {
-       return ((char *)prop)[0] != 0;
+       if (*ptr != 0)
+               return NULL;
+       return (struct prop_hdr *)ptr;
 }
 
 #ifndef NDEBUG
@@ -174,8 +178,11 @@ static struct tal_hdr *to_tal_hdr(const void *ctx)
        check_bounds(ignore_destroying_bit(t->parent_child));
        check_bounds(t->list.next);
        check_bounds(t->list.prev);
-       if (t->prop && !is_literal(t->prop))
-               check_bounds(t->prop);
+       if (t->prop) {
+               struct prop_hdr *p = is_prop_hdr(t->prop);
+               if (p)
+                       check_bounds(p);
+       }
        return t;
 }
 
@@ -215,13 +222,12 @@ static void notify(const struct tal_hdr *ctx,
                   enum tal_notify_type type, const void *info,
                   int saved_errno)
 {
-        const struct prop_hdr *p;
+        const char *ptr;
+       const struct prop_hdr *p;
 
-        for (p = ctx->prop; p; p = p->next) {
+        for (ptr = ctx->prop; ptr && (p = is_prop_hdr(ptr)) != NULL; ptr = p->next) {
                struct notifier *n;
 
-                if (is_literal(p))
-                       break;
                 if (p->type != NOTIFIER)
                        continue;
                n = (struct notifier *)p;
@@ -255,29 +261,54 @@ static void *allocate(size_t size)
        return ret;
 }
 
-static struct prop_hdr **find_property_ptr(const struct tal_hdr *t,
-                                          enum prop_type type)
+/* Returns a pointer to the pointer: can cast (*ret) to a (struct prop_ptr *) */
+static char **find_property_ptr(struct tal_hdr *t, enum prop_type type)
 {
-        struct prop_hdr **p;
+       char **ptr;
+        struct prop_hdr *p;
 
-        for (p = (struct prop_hdr **)&t->prop; *p; p = &(*p)->next) {
-                if (is_literal(*p)) {
-                        if (type == NAME)
-                                return p;
-                        break;
-                }
-                if ((*p)->type == type)
-                        return p;
-        }
-        return NULL;
+       /* NAME is special, as it can be a literal: see find_name_property */
+       assert(type != NAME);
+       for (ptr = &t->prop; *ptr; ptr = &p->next) {
+               if (!is_prop_hdr(*ptr))
+                       break;
+               p = (struct prop_hdr *)*ptr;
+                if (p->type == type)
+                        return ptr;
+       }
+       return NULL;
+}
+
+/* This is special:
+ * NULL - not found
+ * *literal: true - char **, pointer to literal pointer.
+ * *literal: false - struct prop_hdr **, pointer to header ptr.
+ */
+static char **find_name_property(struct tal_hdr *t, bool *literal)
+{
+       char **ptr;
+        struct prop_hdr *p;
+
+       for (ptr = &t->prop; *ptr; ptr = &p->next) {
+               if (!is_prop_hdr(*ptr)) {
+                       *literal = true;
+                       return ptr;
+               }
+               p = (struct prop_hdr *)*ptr;
+                if (p->type == NAME) {
+                       *literal = false;
+                        return ptr;
+               }
+       }
+       return NULL;
 }
 
-static void *find_property(const struct tal_hdr *parent, enum prop_type type)
+static void *find_property(struct tal_hdr *parent, enum prop_type type)
 {
-        struct prop_hdr **p = find_property_ptr(parent, type);
+        char **ptr = find_property_ptr(parent, type);
 
-        if (p)
-                return *p;
+        if (ptr)
+                return (struct prop_hdr *)*ptr;
         return NULL;
 }
 
@@ -287,7 +318,7 @@ static void init_property(struct prop_hdr *hdr,
 {
        hdr->type = type;
        hdr->next = parent->prop;
-       parent->prop = hdr;
+       parent->prop = (char *)hdr;
 }
 
 static struct notifier *add_notifier_property(struct tal_hdr *t,
@@ -321,17 +352,20 @@ static enum tal_notify_type del_notifier_property(struct tal_hdr *t,
                                                  bool match_extra_arg,
                                                  void *extra_arg)
 {
-        struct prop_hdr **p;
+       char **ptr;
+       struct prop_hdr *p;
 
-        for (p = (struct prop_hdr **)&t->prop; *p; p = &(*p)->next) {
+       for (ptr = &t->prop; *ptr; ptr = &p->next) {
                struct notifier *n;
                enum tal_notify_type types;
 
-                if (is_literal(*p))
+               p = is_prop_hdr(*ptr);
+               if (!p)
                        break;
-                if ((*p)->type != NOTIFIER)
+
+                if (p->type != NOTIFIER)
                        continue;
-               n = (struct notifier *)*p;
+               n = (struct notifier *)p;
                if (n->u.notifyfn != fn)
                        continue;
 
@@ -341,8 +375,8 @@ static enum tal_notify_type del_notifier_property(struct tal_hdr *t,
                    && extra_arg != EXTRA_ARG(n))
                        continue;
 
-               *p = (*p)->next;
-               freefn(n);
+               *ptr = p->next;
+               freefn(p);
                return types & ~(NOTIFY_IS_DESTRUCTOR|NOTIFY_EXTRA_ARG);
         }
         return 0;
@@ -388,7 +422,8 @@ static bool add_child(struct tal_hdr *parent, struct tal_hdr *child)
 
 static void del_tree(struct tal_hdr *t, const tal_t *orig, int saved_errno)
 {
-       struct prop_hdr **prop, *p, *next;
+       struct prop_hdr *prop;
+       char *ptr, *next;
 
        assert(!taken(from_tal_hdr(t)));
 
@@ -402,10 +437,10 @@ static void del_tree(struct tal_hdr *t, const tal_t *orig, int saved_errno)
        notify(t, TAL_NOTIFY_FREE, (tal_t *)orig, saved_errno);
 
        /* Now free children and groups. */
-       prop = find_property_ptr(t, CHILDREN);
+       prop = find_property(t, CHILDREN);
        if (prop) {
                struct tal_hdr *i;
-               struct children *c = (struct children *)*prop;
+               struct children *c = (struct children *)prop;
 
                while ((i = list_top(&c->children, struct tal_hdr, list))) {
                        list_del(&i->list);
@@ -414,9 +449,9 @@ static void del_tree(struct tal_hdr *t, const tal_t *orig, int saved_errno)
        }
 
         /* Finally free our properties. */
-        for (p = t->prop; p && !is_literal(p); p = next) {
-                next = p->next;
-               freefn(p);
+       for (ptr = t->prop; ptr && (prop = is_prop_hdr(ptr)); ptr = next) {
+                next = prop->next;
+               freefn(ptr);
         }
         freefn(t);
 }
@@ -590,25 +625,34 @@ bool tal_del_destructor2_(const tal_t *ctx, void (*destroy)(void *me, void *arg)
 bool tal_set_name_(tal_t *ctx, const char *name, bool literal)
 {
         struct tal_hdr *t = debug_tal(to_tal_hdr(ctx));
-        struct prop_hdr **prop = find_property_ptr(t, NAME);
+       bool was_literal;
+       char **nptr;
 
         /* Get rid of any old name */
-        if (prop) {
-                struct name *oldname = (struct name *)*prop;
-                if (is_literal(&oldname->hdr))
-                        *prop = NULL;
-                else {
-                        *prop = oldname->hdr.next;
+       nptr = find_name_property(t, &was_literal);
+       if (nptr) {
+               if (was_literal)
+                       *nptr = NULL;
+               else {
+                       struct name *oldname;
+
+                       oldname = (struct name *)*nptr;
+                       *nptr = oldname->hdr.next;
                        freefn(oldname);
-                }
+               }
         }
 
         if (literal && name[0]) {
-                struct prop_hdr **p;
+               char **ptr;
+               struct prop_hdr *prop;
 
                 /* Append literal. */
-                for (p = &t->prop; *p && !is_literal(*p); p = &(*p)->next);
-                *p = (struct prop_hdr *)name;
+               for (ptr = &t->prop; *ptr; ptr = &prop->next) {
+                       prop = is_prop_hdr(*ptr);
+                       if (!prop)
+                               break;
+               }
+                *ptr = (char *)name;
         } else if (!add_name_property(t, name))
                return false;
 
@@ -620,15 +664,16 @@ bool tal_set_name_(tal_t *ctx, const char *name, bool literal)
 
 const char *tal_name(const tal_t *t)
 {
-        struct name *n;
+       char **nptr;
+       bool literal;
 
-       n = find_property(debug_tal(to_tal_hdr(t)), NAME);
-       if (!n)
+       nptr = find_name_property(debug_tal(to_tal_hdr(t)), &literal);
+       if (!nptr)
                return NULL;
+       if (literal)
+               return *nptr;
 
-       if (is_literal(&n->hdr))
-               return (const char *)n;
-       return n->name;
+       return ((struct name *)(*nptr))->name;
 }
 
 size_t tal_bytelen(const tal_t *ptr)
@@ -832,36 +877,38 @@ void tal_set_backend(void *(*alloc_fn)(size_t size),
 static void dump_node(unsigned int indent, const struct tal_hdr *t)
 {
        unsigned int i;
-        const struct prop_hdr *p;
+        const struct prop_hdr *prop;
+       const char *ptr;
 
        for (i = 0; i < indent; i++)
                fprintf(stderr, "  ");
        fprintf(stderr, "%p len=%zu", t, t->bytelen);
-        for (p = t->prop; p; p = p->next) {
+        for (ptr = t->prop; ptr; ptr = prop->next) {
                struct children *c;
                struct name *n;
                struct notifier *no;
-                if (is_literal(p)) {
-                       fprintf(stderr, " \"%s\"", (const char *)p);
+                prop = is_prop_hdr(ptr);
+               if (!prop) {
+                       fprintf(stderr, " \"%s\"", ptr);
                        break;
                }
-               switch (p->type) {
+               switch (prop->type) {
                case CHILDREN:
-                       c = (struct children *)p;
+                       c = (struct children *)prop;
                        fprintf(stderr, " CHILDREN(%p):parent=%p,children={%p,%p}",
-                              p, c->parent,
+                              prop, c->parent,
                               c->children.n.prev, c->children.n.next);
                        break;
                case NAME:
-                       n = (struct name *)p;
-                       fprintf(stderr, " NAME(%p):%s", p, n->name);
+                       n = (struct name *)prop;
+                       fprintf(stderr, " NAME(%p):%s", prop, n->name);
                        break;
                case NOTIFIER:
-                       no = (struct notifier *)p;
-                       fprintf(stderr, " NOTIFIER(%p):fn=%p", p, no->u.notifyfn);
+                       no = (struct notifier *)prop;
+                       fprintf(stderr, " NOTIFIER(%p):fn=%p", prop, no->u.notifyfn);
                        break;
                default:
-                       fprintf(stderr, " **UNKNOWN(%p):%i**", p, p->type);
+                       fprintf(stderr, " **UNKNOWN(%p):%i**", prop, prop->type);
                }
        }
        fprintf(stderr, "\n");
@@ -873,7 +920,7 @@ static void tal_dump_(unsigned int level, const struct tal_hdr *t)
 
        dump_node(level, t);
 
-       children = find_property(t, CHILDREN);
+       children = find_property((struct tal_hdr *)t, CHILDREN);
        if (children) {
                struct tal_hdr *i;
 
@@ -904,7 +951,8 @@ static bool check_err(struct tal_hdr *t, const char *errorstr,
 static bool check_node(struct children *parent_child,
                       struct tal_hdr *t, const char *errorstr)
 {
-       struct prop_hdr *p;
+       struct prop_hdr *prop;
+       char *p;
        struct name *name = NULL;
        struct children *children = NULL;
 
@@ -914,23 +962,24 @@ static bool check_node(struct children *parent_child,
        if (ignore_destroying_bit(t->parent_child) != parent_child)
                return check_err(t, errorstr, "incorrect parent");
 
-       for (p = t->prop; p; p = p->next) {
-               if (is_literal(p)) {
+       for (p = t->prop; p; p = prop->next) {
+               prop = is_prop_hdr(p);
+               if (!prop) {
                        if (name)
                                return check_err(t, errorstr,
                                                 "has extra literal");
                        break;
                }
-               if (!in_bounds(p))
+               if (!in_bounds(prop))
                        return check_err(t, errorstr,
                                         "has bad property pointer");
 
-               switch (p->type) {
+               switch (prop->type) {
                case CHILDREN:
                        if (children)
                                return check_err(t, errorstr,
                                                 "has two child nodes");
-                       children = (struct children *)p;
+                       children = (struct children *)prop;
                        break;
                case NOTIFIER:
                        break;
@@ -938,7 +987,7 @@ static bool check_node(struct children *parent_child,
                        if (name)
                                return check_err(t, errorstr,
                                                 "has two names");
-                       name = (struct name *)p;
+                       name = (struct name *)prop;
                        break;
                default:
                        return check_err(t, errorstr, "has unknown property");

diff --git a/ccan/tal/test/run-notifier.c b/ccan/tal/test/run-notifier.c
index 150f00adae9e2d60c0df655ff2ac105e34c2c85a..47e436408cbe4ce803951eabb59410e2d6fde1d2 100644 (file)
--- a/ccan/tal/test/run-notifier.c
+++ b/ccan/tal/test/run-notifier.c
@@ -13,8 +13,8 @@ static void *my_realloc(void *old, size_t size)
        void *new = realloc(old, size);
        if (new == old) {
                void *p = malloc(size);
-               memcpy(p, old, size);
-               free(old);
+               memcpy(p, new, size);
+               free(new);
                new = p;
        }
        return new;