peer_control: Fix a use-after-free bug.

[ZmnSCPxj]

Possibly related to #1235 and #1236, but not confirmed to fix that.

[rustyrussell]

I don't see where we free channel here?

[ZmnSCPxj]

We are freeing error at the end of the function. In one path, error is loaded from channel->error. Since the function frees error, the channel->error becomes a pointer to a freed memory.

[cdecker]

Excellent find, we are indeed leaving a dangling pointer in channel->error. Wondering however if a cleaner fix would be to allocate the error in here off of tmpctx and rely on the auto-free of that context:

lightning/lightningd/peer_control.c

Lines 440 to 443 in 234d67d

	error = peer_accept_channel(ld, id, addr, cs, gossip_index,
	gfeatures, lfeatures,
	peer_fd, gossip_fd, channel_id,
	in_msg);

That would avoid duplicating the error.

[ZmnSCPxj]

Allocated off in_msg the "Weird request" case instead, and removed the tal_free.

[ZmnSCPxj]

Allocating off in_msg fails CI https://travis-ci.org/ElementsProject/lightning/jobs/355149369, unfortunately I cannot get the details since the rerun breaks completely and I cannot extract the failure cause. Reverted to the previous "make a copy of channel->error" instead. The "allocate off in_msg" version is in my branch https://github.com/ZmnSCPxj/lightning/tree/other-peer-nongossip-use-after-free if anyone wants to take a look at it.

[rustyrussell]

Here's the clue from that failure:
'result': {'leaks': [{'value': '0x9d95960', 'parents': ['wire/gen_peer_wire.c:608:u8[]', 'lightningd/lightningd.c:48:struct lightningd'], 'backtrace': ['ccan/ccan/tal/tal.c:467 (tal_alloc_)', 'ccan/ccan/tal/tal.c:496 (tal_alloc_arr_)', 'ccan/ccan/tal/str/str.c:90 (tal_vfmt)', 'ccan/ccan/tal/str/str.c:45 (tal_fmt)', 'common/wire_error.c:80 (sanitize_error)', 'lightningd/peer_control.c:469 (peer_sent_nongossip)', 'lightningd/gossip_control.c:59 (peer_nongossip)', 'lightningd/gossip_control.c:168 (gossip_msg)', 'lightningd/subd.c:504 (sd_msg_read)', 'lightningd/subd.c:330 (read_fds)', 'ccan/ccan/io/io.c:59 (next_plan)', 'ccan/ccan/io/io.c:387 (do_plan)', 'ccan/ccan/io/io.c:397 (io_ready)', 'ccan/ccan/io/poll.c:310 (io_loop)', 'lightningd/lightningd.c:400 (main)'], 'label': 'ccan/ccan/tal/str/str.c:90:char[]'}, {'value': '0x9d76378', 'parents': ['wire/gen_peer_wire.c:608:u8[]', 'lightningd/lightningd.c:48:struct lightningd'], 'backtrace': ['ccan/ccan/tal/tal.c:467 (tal_alloc_)', 'ccan/ccan/tal/tal.c:496 (tal_alloc_arr_)', 'wire/gen_peer_wire.c:78 (fromwire_error)', 'common/wire_error.c:60 (sanitize_error)', 'lightningd/peer_control.c:469 (peer_sent_nongossip)', 'lightningd/gossip_control.c:59 (peer_nongossip)', 'lightningd/gossip_control.c:168 (gossip_msg)', 'lightningd/subd.c:504 (sd_msg_read)', 'lightningd/subd.c:330 (read_fds)', 'ccan/ccan/io/io.c:59 (next_plan)', 'ccan/ccan/io/io.c:387 (do_plan)', 'ccan/ccan/io/io.c:397 (io_ready)', 'ccan/ccan/io/poll.c:310 (io_loop)', 'lightningd/lightningd.c:400 (main)'], 'label': 'wire/gen_peer_wire.c:78:u8[]'}, {'value': '0x9d95700', 'parents': ['wire/gen_peer_wire.c:608:u8[]', 'lightningd/lightningd.c:48:struct lightningd'], 'backtrace': ['ccan/ccan/tal/tal.c:467 (tal_alloc_)', 'ccan/ccan/tal/tal.c:496 (tal_alloc_arr_)', 'common/utils.c:11 (tal_hexstr)', 'wire/fromwire.c:229 (fmt_channel_id_)', 'common/type_to_string.c:35 (type_to_string_)', 'common/wire_error.c:80 (sanitize_error)', 'lightningd/peer_control.c:469 (peer_sent_nongossip)', 'lightningd/gossip_control.c:59 (peer_nongossip)', 'lightningd/gossip_control.c:168 (gossip_msg)', 'lightningd/subd.c:504 (sd_msg_read)', 'lightningd/subd.c:330 (read_fds)', 'ccan/ccan/io/io.c:59 (next_plan)', 'ccan/ccan/io/io.c:387 (do_plan)', 'ccan/ccan/io/io.c:397 (io_ready)', 'ccan/ccan/io/poll.c:310 (io_loop)', 'lightningd/lightningd.c:400 (main)'], 'label': 'common/utils.c:11:char[]'}]}}

Basically, we call sanitize_error() and allocate that off error which is actually not freed any more.

Christian's advice of using tmpctx here is correct: one subtle advantage of tal() is that you can have two paths, one of which uses a temporary and one which doesn't, and simply free the parent and not worry about distinguishing the two. In this case, tmpctx is the correct parent.

[rustyrussell]

How's that, @ZmnSCPxj ?

[ZmnSCPxj]

Looks OK.

[practicalswift]

Oh, very nice find!