From 5a520f4a0767e318cca5c71b48d3ee68d70529ec Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 12 Jun 2019 10:08:55 +0930 Subject: [PATCH] plugin: don't call notification after free. This is an old bug, where a plugin can get called while we're shutting down (and have freed plugins), but it's triggered more reliably by the new warning notification hook. For good measure, we also make freeing a plugin self-delete. Valgrind error file: valgrind-errors.16763 ==16886== Invalid read of size 8 ==16886== at 0x422919: plugins_notify (plugin.c:1096) ==16886== by 0x413919: notify_warning (notification.c:61) ==16886== by 0x412BDE: logv (log.c:251) ==16886== by 0x412A98: log_ (log.c:311) ==16886== by 0x4044BE: bcli_finished (bitcoind.c:178) ==16886== by 0x459480: destroy_conn (poll.c:244) ==16886== by 0x459499: destroy_conn_close_fd (poll.c:250) ==16886== by 0x4619E1: notify (tal.c:235) ==16886== by 0x461A7E: del_tree (tal.c:397) ==16886== by 0x461AB5: del_tree (tal.c:407) ==16886== by 0x461AB5: del_tree (tal.c:407) ==16886== by 0x461AB5: del_tree (tal.c:407) ==16886== Address 0x634a578 is 40 bytes inside a block of size 352 free'd ==16886== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==16886== by 0x461AFD: del_tree (tal.c:416) ==16886== by 0x461FB7: tal_free (tal.c:481) ==16886== by 0x411E0A: main (lightningd.c:841) ==16886== Block was alloc'd at ==16886== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==16886== by 0x4617CE: allocate (tal.c:245) ==16886== by 0x461E4C: tal_alloc_ (tal.c:423) ==16886== by 0x42255E: plugins_new (plugin.c:106) ==16886== by 0x41133D: new_lightningd (lightningd.c:218) ==16886== by 0x411AD4: main (lightningd.c:649) Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 3 ++- lightningd/plugin.c | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index d144dec99..583726091 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -838,7 +838,8 @@ int main(int argc, char *argv[]) shutdown_subdaemons(ld); - tal_free(ld->plugins); + /* Remove plugins. */ + ld->plugins = tal_free(ld->plugins); /* Clean up the JSON-RPC. This needs to happen in a DB transaction since * it might actually be touching the DB in some destructors, e.g., diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 7fed171b0..829d1b24c 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -112,6 +112,11 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, return p; } +static void destroy_plugin(struct plugin *p) +{ + list_del(&p->list); +} + void plugin_register(struct plugins *plugins, const char* path TAKES) { struct plugin *p; @@ -126,6 +131,7 @@ void plugin_register(struct plugins *plugins, const char* path TAKES) path_basename(tmpctx, p->cmd)); p->methods = tal_arr(p, const char *, 0); list_head_init(&p->plugin_opts); + tal_add_destructor(p, destroy_plugin); } static bool paths_match(const char *cmd, const char *name) @@ -1093,9 +1099,14 @@ void plugins_notify(struct plugins *plugins, const struct jsonrpc_notification *n TAKES) { struct plugin *p; - list_for_each(&plugins->plugins, p, list) { - if (plugin_subscriptions_contains(p, n->method)) - plugin_send(p, json_stream_dup(p, n->stream, p->log)); + + /* If we're shutting down, ld->plugins will be NULL */ + if (plugins) { + list_for_each(&plugins->plugins, p, list) { + if (plugin_subscriptions_contains(p, n->method)) + plugin_send(p, json_stream_dup(p, n->stream, + p->log)); + } } if (taken(n)) tal_free(n);