From 55510ea27a798aa35af4ce999d4829dcce891263 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 19 May 2017 20:33:28 +0930 Subject: [PATCH] io_write_wire: always make a copy (or steal if take). I caught the gossip daemon freeing a message, while it was queued to be written. Using tal_dup_arr() is the Right Thing, as it handles taken() properly automatically. ------------------------------- Valgrind errors -------------------------------- Valgrind error file: /tmp/lightning-rvc7d5oi/test_forward/lightning-3/valgrind-errors ==11057== Invalid read of size 8 ==11057== at 0x1328F2: to_tal_hdr (tal.c:174) ==11057== by 0x133894: tal_len (tal.c:659) ==11057== by 0x11BBE7: do_write_wire (wire_io.c:103) ==11057== by 0x127B95: do_plan (io.c:369) ==11057== by 0x127C31: io_ready (io.c:390) ==11057== by 0x129461: io_loop (poll.c:295) ==11057== by 0x10CBB4: main (gossip.c:722) ==11057== Address 0x55a99d8 is 24 bytes inside a block of size 200 free'd ==11057== at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==11057== by 0x133000: del_tree (tal.c:416) ==11057== by 0x132F77: del_tree (tal.c:405) ==11057== by 0x13333E: tal_free (tal.c:504) ==11057== by 0x1123F1: queue_broadcast (broadcast.c:38) ==11057== by 0x111EB0: handle_node_announcement (routing.c:918) ==11057== by 0x10B166: handle_gossip_msg (gossip.c:170) ==11057== by 0x10B76B: owner_msg_in (gossip.c:335) ==11057== by 0x12712E: next_plan (io.c:59) ==11057== by 0x127BD0: do_plan (io.c:376) ==11057== by 0x127C09: io_ready (io.c:386) ==11057== by 0x129461: io_loop (poll.c:295) ==11057== Block was alloc'd at ==11057== at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==11057== by 0x132AE7: allocate (tal.c:245) ==11057== by 0x1330A3: tal_alloc_ (tal.c:443) ==11057== by 0x1332A6: tal_alloc_arr_ (tal.c:491) ==11057== by 0x133FEC: tal_dup_ (tal.c:846) ==11057== by 0x112347: new_queued_message (broadcast.c:20) ==11057== by 0x11240B: queue_broadcast (broadcast.c:43) ==11057== by 0x111EB0: handle_node_announcement (routing.c:918) ==11057== by 0x10B166: handle_gossip_msg (gossip.c:170) ==11057== by 0x10B76B: owner_msg_in (gossip.c:335) ==11057== by 0x12712E: next_plan (io.c:59) ==11057== by 0x127BD0: do_plan (io.c:376) Signed-off-by: Rusty Russell wire_io: make a copy in io_write_wire (unless taken()). I hit a corner case where gossipd freed a duplicate while it was being sent out; this kind of thing doesn't happen if io_write_wire() makes a copy by default. We also do a memcheck() here; this gives us a caller in the backtrace if there are uninitialized bytes, rather than waiting until the write which happens later. Signed-off-by: Rusty Russell --- wire/wire_io.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/wire/wire_io.c b/wire/wire_io.c index 5e5fa020c..57f338309 100644 --- a/wire/wire_io.c +++ b/wire/wire_io.c @@ -1,6 +1,7 @@ #include /* FIXME: io_plan needs size_t */ #include +#include #include #include #include @@ -115,8 +116,7 @@ static int do_write_wire(int fd, struct io_plan_arg *arg) if (arg->u2.s != totlen) return 0; - if (taken(arg->u1.cp)) - tal_free(arg->u1.cp); + tal_free(arg->u1.cp); return 1; } @@ -128,7 +128,8 @@ struct io_plan *io_write_wire_(struct io_conn *conn, { struct io_plan_arg *arg = io_plan_arg(conn, IO_OUT); - arg->u1.const_vp = data; + arg->u1.const_vp = tal_dup_arr(conn, u8, memcheck(data, tal_len(data)), + tal_len(data), 0); /* We use u2 to store the length we've written. */ arg->u2.s = INSIDE_HEADER_BIT;