From 89d143bc63ea282d5b905f1baa1aa1f1f0cc0973 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Oct 2021 06:49:32 +0930 Subject: [PATCH] lightningd: fix use-after-free during shutdown. When we are calling hooks, we track them via a linked list. As they execute, we pop them off the list in plugin_hook_killed(). When we kill a plugin, we have a destructor which remove its entry from the linked list: plugin_hook_killed. If it's at the head of the list, that means the plugin died while processing the hook, so instead of just deleting it, we call plugin_hook_killed() which behaves as if it said "result: continue". But plugin_hook_killed() just returns if we're shutting down; this leaves the link (then freed) on the list, and the *next* plugin tries to unlink from the list, accessing the previous free entry. The fix is simple: unlink from the list in plugin_hook_killed() even if we're shutting down. ``` Valgrind error file: valgrind-errors.78570 ==78570== Invalid write of size 8 ==78570== at 0x174B55: list_del_ (list.h:328) ==78570== by 0x174FCC: plugin_hook_killed (plugin_hook.c:135) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== by 0x21E1A8: del_tree (tal.c:412) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x16EBD1: plugin_kill (plugin.c:345) ==78570== by 0x16F9C4: plugin_conn_finish (plugin.c:724) ==78570== by 0x20F1A5: destroy_conn (poll.c:244) ==78570== by 0x20F1C9: destroy_conn_close_fd (poll.c:250) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== Address 0x6aee688 is 40 bytes inside a block of size 72 free'd ==78570== at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==78570== by 0x21E224: del_tree (tal.c:421) ==78570== by 0x21E1A8: del_tree (tal.c:412) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x16EBD1: plugin_kill (plugin.c:345) ==78570== by 0x16F9C4: plugin_conn_finish (plugin.c:724) ==78570== by 0x20F1A5: destroy_conn (poll.c:244) ==78570== by 0x20F1C9: destroy_conn_close_fd (poll.c:250) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x20D7B6: io_close (io.c:450) ==78570== Block was alloc'd at ==78570== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==78570== by 0x21DCAD: allocate (tal.c:250) ==78570== by 0x21E26E: tal_alloc_ (tal.c:428) ==78570== by 0x175599: plugin_hook_call_ (plugin_hook.c:259) ==78570== by 0x13616F: plugin_hook_call_onion_message_blinded (onion_message.c:126) ==78570== by 0x13643B: handle_obs_onionmsg_to_us (onion_message.c:187) ==78570== by 0x138BBD: gossip_msg (gossip_control.c:140) ==78570== by 0x178AEC: sd_msg_read (subd.c:495) ==78570== by 0x20CA00: next_plan (io.c:59) ==78570== by 0x20D608: do_plan (io.c:407) ==78570== by 0x20D64A: io_ready (io.c:417) ==78570== by 0x20F8F1: io_loop (poll.c:445) ``` Signed-off-by: Rusty Russell --- lightningd/plugin_hook.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lightningd/plugin_hook.c b/lightningd/plugin_hook.c index b6643e7d7..db033428e 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -157,19 +157,20 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks, struct plugin_hook_call_link *last, *it; bool in_transaction = false; - log_debug(r->ld->log, "Plugin %s returned from %s hook call", - r->plugin->shortname, r->hook->name); + /* Pop the head off the call chain and continue with the next */ + last = list_pop(&r->call_chain, struct plugin_hook_call_link, list); + assert(last != NULL); + tal_del_destructor(last, plugin_hook_killed); + tal_free(last); if (r->ld->state == LD_STATE_SHUTDOWN) { log_debug(r->ld->log, "Abandoning plugin hook call due to shutdown"); return; } - /* Pop the head off the call chain and continue with the next */ - last = list_pop(&r->call_chain, struct plugin_hook_call_link, list); - assert(last != NULL); - tal_del_destructor(last, plugin_hook_killed); - tal_free(last); + + log_debug(r->ld->log, "Plugin %s returned from %s hook call", + r->plugin->shortname, r->hook->name); if (buffer) { resulttok = json_get_member(buffer, toks, "result");