tal: don't defer-after-free if a destructor deletes itself.

==10868==    at 0x109A96: notify (tal.c:220)
==10868==    by 0x109F7E: del_tree (tal.c:397)
==10868==    by 0x10A31A: tal_free (tal.c:481)
==10868==    by 0x10BE73: main (run-destructor.c:75)
==10868==  Address 0x4a60bd8 is 8 bytes inside a block of size 32 free'd
==10868==    at 0x483BA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==10868==    by 0x109D4F: del_notifier_property (tal.c:340)
==10868==    by 0x10A610: tal_del_notifier_ (tal.c:564)
==10868==    by 0x10A687: tal_del_destructor_ (tal.c:576)
==10868==    by 0x10B653: remove_own_destructor (run-destructor.c:35)
==10868==    by 0x109A67: notify (tal.c:235)
==10868==    by 0x109F7E: del_tree (tal.c:397)
==10868==    by 0x10A31A: tal_free (tal.c:481)
==10868==    by 0x10BE73: main (run-destructor.c:75)
==10868==  Block was alloc'd at
==10868==    at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==10868==    by 0x109AD5: allocate (tal.c:245)
==10868==    by 0x109C3E: add_notifier_property (tal.c:300)
==10868==    by 0x10A467: tal_add_destructor_ (tal.c:516)
==10868==    by 0x10BDFE: main (run-destructor.c:74)

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

diff --git a/ccan/tal/tal.c b/ccan/tal/tal.c
index dff0dcfba2058b0815f79423c0798af9b806fb67..a49541114ff863128dd03a1429fa84c1d4e06ffb 100644 (file)
--- a/ccan/tal/tal.c
+++ b/ccan/tal/tal.c
@@ -53,7 +53,7 @@ struct name {
 struct notifier {
        struct prop_hdr hdr; /* NOTIFIER */
        enum tal_notify_type types;
-       union {
+       union notifier_cb {
                void (*notifyfn)(tal_t *, enum tal_notify_type, void *);
                void (*destroy)(tal_t *); /* If NOTIFY_IS_DESTRUCTOR set */
                void (*destroy2)(tal_t *, void *); /* If NOTIFY_EXTRA_ARG */
@@ -228,11 +228,16 @@ static void notify(const struct tal_hdr *ctx,
                if (n->types & type) {
                        errno = saved_errno;
                        if (n->types & NOTIFY_IS_DESTRUCTOR) {
+                               /* Blatt this notifier in case it tries to
+                                * tal_del_destructor() from inside */
+                               union notifier_cb cb = n->u;
+                               /* It's a union, so this NULLs destroy2 too! */
+                               n->u.destroy = NULL;
                                if (n->types & NOTIFY_EXTRA_ARG)
-                                       n->u.destroy2(from_tal_hdr(ctx),
-                                                     EXTRA_ARG(n));
+                                       cb.destroy2(from_tal_hdr(ctx),
+                                                   EXTRA_ARG(n));
                                else
-                                       n->u.destroy(from_tal_hdr(ctx));
+                                       cb.destroy(from_tal_hdr(ctx));
                        } else
                                n->u.notifyfn(from_tal_hdr_or_null(ctx), type,
                                              (void *)info);

diff --git a/ccan/tal/tal.h b/ccan/tal/tal.h
index e25dcb92985922acc5a50007f5c397ddb4aa9237..8b7ffca5ea8125ef8ba36a623d989e8aad6fff33 100644 (file)
--- a/ccan/tal/tal.h
+++ b/ccan/tal/tal.h
@@ -164,7 +164,8 @@ void *tal_free(const tal_t *p);
  * @function: the function to call before it's freed.
  *
  * If @function has not been successfully added as a destructor, this returns
- * false.
+ * false.  Note that if we're inside the destructor call itself, this will
+ * return false.
  */
 #define tal_del_destructor(ptr, function)                                    \
        tal_del_destructor_((ptr), typesafe_cb(void, void *, (function), (ptr)))
@@ -195,7 +196,8 @@ void *tal_free(const tal_t *p);
  * @function: the function to call before it's freed.
  *
  * If @function has not been successfully added as a destructor, this returns
- * false.
+ * false.  Note that if we're inside the destructor call itself, this will
+ * return false.
  */
 #define tal_del_destructor(ptr, function)                                    \
        tal_del_destructor_((ptr), typesafe_cb(void, void *, (function), (ptr)))

diff --git a/ccan/tal/test/run-destructor.c b/ccan/tal/test/run-destructor.c
index 7183f7c5b5a3daaff2e543d35e9f25c6d33366a2..e3d0c0d88557319d3e87e7bade6073a12979d8cc 100644 (file)
--- a/ccan/tal/test/run-destructor.c
+++ b/ccan/tal/test/run-destructor.c
@@ -29,11 +29,17 @@ static void destroy_inc(char *p UNNEEDED)
        destroy_count++;
 }
 
+static void remove_own_destructor(char *p)
+{
+       destroy_count++;
+       ok1(!tal_del_destructor(p, remove_own_destructor));
+}
+
 int main(void)
 {
        char *child2;
 
-       plan_tests(18);
+       plan_tests(21);
 
        destroy_count = 0;
        parent = tal(NULL, char);
@@ -63,6 +69,12 @@ int main(void)
        tal_free(parent);
        ok1(destroy_count == 4);
 
+       destroy_count = 0;
+       parent = tal(NULL, char);
+       ok1(tal_add_destructor(parent, remove_own_destructor));
+       tal_free(parent);
+       ok1(destroy_count == 1);
+       
        tal_cleanup();
        return exit_status();
 }

diff --git a/ccan/timer/timer.c b/ccan/timer/timer.c
index 2685d3efde96f780fbbc3dc946d6acc4b9def9e1..48bded19656456a500703d77d509ba750053bbda 100644 (file)
--- a/ccan/timer/timer.c
+++ b/ccan/timer/timer.c
@@ -97,7 +97,7 @@ void timer_init(struct timer *t)
        list_node_init(&t->list);
 }
 
-static bool list_node_initted(const struct list_node *n)
+static inline bool list_node_initted(const struct list_node *n)
 {
        return n->prev == n;
 }