From 82ed71d621c1f88f9c059cf0cc83cd00ffa6b11e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 28 Jan 2021 10:06:49 +1030 Subject: [PATCH] connectd: don't crash if connect() fails immediately. Took me a while (stressing under valgrind) to reproduce this, then longer to figure out how it happened. Turns out io_new_conn() can fail if the init function fails. In our case, this can happen if connect() immediately returns an error (inside io_connect). But we've already set the finish function, which (if this was the last address), will free connect, making the assignment `connect->conn = ...` write to a freed address. Either way, if it fails, try_connect_one_addr() has taken care to update connect->conn, or free connect, and the caller should not do it. Here's the valgrind trace: ``` ==384981== Invalid write of size 8 ==384981== at 0x11127C: try_connect_one_addr (connectd.c:880) ==384981== by 0x112BA1: destroy_io_conn (connectd.c:708) ==384981== by 0x141459: destroy_conn (poll.c:244) ==384981== by 0x14147F: destroy_conn_close_fd (poll.c:250) ==384981== by 0x149EB9: notify (tal.c:240) ==384981== by 0x149F8B: del_tree (tal.c:402) ==384981== by 0x14A51A: tal_free (tal.c:486) ==384981== by 0x140036: io_close (io.c:450) ==384981== by 0x1400B3: do_plan (io.c:401) ==384981== by 0x140134: io_ready (io.c:423) ==384981== by 0x141A57: io_loop (poll.c:445) ==384981== by 0x112CB0: main (connectd.c:1703) ==384981== Address 0x4d67020 is 64 bytes inside a block of size 160 free'd ==384981== at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==384981== by 0x14A020: del_tree (tal.c:421) ==384981== by 0x14A51A: tal_free (tal.c:486) ==384981== by 0x1110C5: try_connect_one_addr (connectd.c:806) ==384981== by 0x112BA1: destroy_io_conn (connectd.c:708) ==384981== by 0x141459: destroy_conn (poll.c:244) ==384981== by 0x14147F: destroy_conn_close_fd (poll.c:250) ==384981== by 0x149EB9: notify (tal.c:240) ==384981== by 0x149F8B: del_tree (tal.c:402) ==384981== by 0x14A51A: tal_free (tal.c:486) ==384981== by 0x140036: io_close (io.c:450) ==384981== by 0x1405DC: io_connect_ (io.c:345) ==384981== Block was alloc'd at ==384981== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==384981== by 0x149CF1: allocate (tal.c:250) ==384981== by 0x14A3C6: tal_alloc_ (tal.c:428) ==384981== by 0x1114F2: try_connect_peer (connectd.c:1526) ==384981== by 0x111717: connect_to_peer (connectd.c:1558) ==384981== by 0x1124F5: recv_req (connectd.c:1627) ==384981== by 0x1188B2: handle_read (daemon_conn.c:31) ==384981== by 0x13FBCB: next_plan (io.c:59) ==384981== by 0x140076: do_plan (io.c:407) ==384981== by 0x140113: io_ready (io.c:417) ==384981== by 0x141A57: io_loop (poll.c:445) ==384981== by 0x112CB0: main (connectd.c:1703) ``` Signed-off-by: Rusty Russell Changelog-Fixed: Occasional crash in connectd due to use-after-free Fixes: #4343 --- connectd/connectd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/connectd/connectd.c b/connectd/connectd.c index 5f889ee94..06cc62af8 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -793,6 +793,7 @@ static void try_connect_one_addr(struct connecting *connect) int fd, af; bool use_proxy = connect->daemon->use_proxy_always; const struct wireaddr_internal *addr = &connect->addrs[connect->addrnum]; + struct io_conn *conn; /* In case we fail without a connection, make destroy_io_conn happy */ connect->conn = NULL; @@ -875,9 +876,14 @@ static void try_connect_one_addr(struct connecting *connect) /* This creates the new connection using our fd, with the initialization * function one of the above. */ if (use_proxy) - connect->conn = io_new_conn(connect, fd, conn_proxy_init, connect); + conn = io_new_conn(connect, fd, conn_proxy_init, connect); else - connect->conn = io_new_conn(connect, fd, conn_init, connect); + conn = io_new_conn(connect, fd, conn_init, connect); + + /* Careful! io_new_conn can fail (immediate connect() failure), and + * that frees connect. */ + if (conn) + connect->conn = conn; } /*~ connectd is responsible for incoming connections, but it's the process of