From c910bdce167ff42aa6d9e4f1b8f905a76f0b9e75 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 17 Feb 2020 10:42:46 +1030 Subject: [PATCH] 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 --- ccan/tal/tal.c | 13 +++++++++---- ccan/tal/tal.h | 6 ++++-- ccan/tal/test/run-destructor.c | 14 +++++++++++++- ccan/timer/timer.c | 2 +- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/ccan/tal/tal.c b/ccan/tal/tal.c index dff0dcfb..a4954111 100644 --- 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 e25dcb92..8b7ffca5 100644 --- 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 7183f7c5..e3d0c0d8 100644 --- 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 2685d3ef..48bded19 100644 --- 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; } -- 2.39.2