From 4d1214b43269f31786b2e71e7915b8a315ee11b0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 14 Jan 2021 10:36:50 +1030 Subject: [PATCH] lightningd: fix double-free when forking subdaemon fails. payload is owned by the peer, which is freed in this case, then we free payload (again). ==1404== Invalid read of size 8 ==1404== at 0x1F39E8: to_tal_hdr (tal.c:174) ==1404== by 0x1F43A4: tal_free (tal.c:479) ==1404== by 0x14B3D1: peer_connected_hook_cb (peer_control.c:1087) ==1404== by 0x15D6E9: plugin_hook_call_ (plugin_hook.c:288) ==1404== by 0x14B40E: plugin_hook_call_peer_connected (peer_control.c:1090) ==1404== by 0x14B5B8: peer_connected (peer_control.c:1135) ==1404== by 0x122FCF: connectd_msg (connect_control.c:310) ==1404== by 0x160291: sd_msg_read (subd.c:480) ==1404== by 0x15FBE7: read_fds (subd.c:308) ==1404== by 0x1E37D1: next_plan (io.c:59) ==1404== by 0x1E434E: do_plan (io.c:407) ==1404== by 0x1E438C: io_ready (io.c:417) ==1404== Address 0x2fcd2268 is 24 bytes inside a block of size 336 free'd ==1404== at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==1404== by 0x1F416E: del_tree (tal.c:421) ==1404== by 0x1F40F2: del_tree (tal.c:412) ==1404== by 0x1F442C: tal_free (tal.c:486) ==1404== by 0x148816: delete_peer (peer_control.c:120) ==1404== by 0x148899: maybe_delete_peer (peer_control.c:136) ==1404== by 0x13A970: destroy_uncommitted_channel (opening_common.c:29) ==1404== by 0x1F3BB1: notify (tal.c:240) ==1404== by 0x1F40A0: del_tree (tal.c:402) ==1404== by 0x1F442C: tal_free (tal.c:486) ==1404== by 0x13D3E9: peer_start_openingd (opening_control.c:911) ==1404== by 0x14B3C2: peer_connected_hook_cb (peer_control.c:1086) ==1404== Block was alloc'd at ==1404== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==1404== by 0x1F3C1B: allocate (tal.c:250) ==1404== by 0x1F41B4: tal_alloc_ (tal.c:428) ==1404== by 0x14B454: peer_connected (peer_control.c:1105) ==1404== by 0x122FCF: connectd_msg (connect_control.c:310) ==1404== by 0x160291: sd_msg_read (subd.c:480) ==1404== by 0x15FBE7: read_fds (subd.c:308) ==1404== by 0x1E37D1: next_plan (io.c:59) ==1404== by 0x1E434E: do_plan (io.c:407) ==1404== by 0x1E438C: io_ready (io.c:417) ==1404== by 0x1E6552: io_loop (poll.c:445) ==1404== by 0x12E2AD: io_loop_with_timers (io_loop_with_timers.c:24) Fixes: #4329 Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index bc3c0bc9f..8fef02b94 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -957,6 +957,11 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS, struct peer *peer = payload->peer; u8 *error; + /* Whatever happens, we free payload (it's currently a child + * of the peer, which may be freed if we fail to start + * subd). */ + tal_steal(tmpctx, payload); + /* If we had a hook, interpret result. */ if (buffer) { const jsmntok_t *resulttok; @@ -977,7 +982,6 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS, buffer + m->start); goto send_error; } - tal_free(payload); return; } else if (!json_tok_streq(buffer, resulttok, "continue")) fatal("Plugin returned an invalid response to the connected " @@ -1031,7 +1035,6 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS, peer_restart_dualopend(peer, payload->pps, channel, NULL); - tal_free(payload); return; #else abort(); @@ -1044,7 +1047,6 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS, channel->peer->addr = addr; peer_start_channeld(channel, payload->pps, NULL, true); - tal_free(payload); return; case CLOSINGD_SIGEXCHANGE: @@ -1053,7 +1055,6 @@ peer_connected_hook_cb(struct peer_connected_hook_payload *payload STEALS, channel->peer->addr = addr; peer_start_closingd(channel, payload->pps, true, NULL); - tal_free(payload); return; } abort(); @@ -1084,7 +1085,6 @@ send_error: } else #endif /* EXPERIMENTAL_FEATURES */ peer_start_openingd(peer, payload->pps, error); - tal_free(payload); } REGISTER_SINGLE_PLUGIN_HOOK(peer_connected,