From 493c2ab1d71e51cfa63be72935b011cdabe2af55 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 27 Nov 2019 13:14:13 +1030 Subject: [PATCH] openingd: clean up and fix minor leak. test_openchannel_hook_1: MEMLEAK: 0x557593c164e8' label=wire/fromwire.c:320:char[]' backtrace:' ccan/ccan/tal/tal.c:437 (tal_alloc_)' ccan/ccan/tal/tal.c:466 (tal_alloc_arr_)' wire/fromwire.c:320 (fromwire_wirestring)' openingd/gen_opening_wire.c:205 (fromwire_opening_got_offer_reply)' openingd/openingd.c:1067 (fundee_channel)' openingd/openingd.c:1279 (handle_peer_in)' openingd/openingd.c:1535 (main)' parents: fromwire_opening_got_offer_reply() allocates two fields off NULL: err_reason and our_upfront_shutdown_script. err_reason is used immediately afterwards (and was the leak detected here), so fixing that is easy. To fix the leak of our_upfront_shutdown_script, it makes sense to simply make it a member of 'state'. Signed-off-by: Rusty Russell --- openingd/openingd.c | 71 +++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/openingd/openingd.c b/openingd/openingd.c index b61a36da8..58f45e7a7 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -97,8 +97,9 @@ struct state { u32 feerate_per_kw; struct bitcoin_txid funding_txid; u16 funding_txout; - /* If set, this is the scriptpubkey they *must* close with */ - u8 *remote_upfront_shutdown_script; + + /* If non-NULL, this is the scriptpubkey we/they *must* close with */ + u8 *upfront_shutdown_script[NUM_SIDES]; /* This is a cluster of fields in open_channel and accept_channel which * indicate the restrictions each side places on the channel. */ @@ -143,6 +144,12 @@ static void negotiation_aborted(struct state *state, bool am_funder, wire_sync_write(REQ_FD, take(msg)); } + /* Default is no shutdown_scriptpubkey: free any leftover ones. */ + state->upfront_shutdown_script[LOCAL] + = tal_free(state->upfront_shutdown_script[LOCAL]); + state->upfront_shutdown_script[REMOTE] + = tal_free(state->upfront_shutdown_script[REMOTE]); + /*~ Reset state. We keep gossipping with them, even though this open * failed. */ memset(&state->channel_id, 0, sizeof(state->channel_id)); @@ -493,9 +500,7 @@ static bool setup_channel_funder(struct state *state) /* We start the 'fund a channel' negotation with the supplied peer, but * stop when we get to the part where we need the funding txid */ -static u8 *funder_channel_start(struct state *state, - u8 *our_upfront_shutdown_script, - u8 channel_flags) +static u8 *funder_channel_start(struct state *state, u8 channel_flags) { u8 *msg; u8 *funding_output_script; @@ -514,8 +519,8 @@ static u8 *funder_channel_start(struct state *state, * - otherwise: * - MAY include a`shutdown_scriptpubkey`. */ - if (!our_upfront_shutdown_script) - our_upfront_shutdown_script = dev_upfront_shutdown_script(tmpctx); + if (!state->upfront_shutdown_script[LOCAL]) + state->upfront_shutdown_script[LOCAL] = dev_upfront_shutdown_script(state); msg = towire_open_channel_option_upfront_shutdown_script(NULL, &chainparams->genesis_blockhash, @@ -536,7 +541,7 @@ static u8 *funder_channel_start(struct state *state, &state->our_points.htlc, &state->first_per_commitment_point[LOCAL], channel_flags, - our_upfront_shutdown_script); + state->upfront_shutdown_script[LOCAL]); sync_crypto_write(state->pps, take(msg)); /* This is usually a very transient state... */ @@ -548,10 +553,6 @@ static u8 *funder_channel_start(struct state *state, if (!msg) return NULL; - /* Default is no shutdown_scriptpubkey: free any leftover one. */ - state->remote_upfront_shutdown_script - = tal_free(state->remote_upfront_shutdown_script); - /* BOLT #2: * * The receiving node MUST fail the channel if: @@ -577,7 +578,7 @@ static u8 *funder_channel_start(struct state *state, &state->their_points.delayed_payment, &state->their_points.htlc, &state->first_per_commitment_point[REMOTE], - &state->remote_upfront_shutdown_script)) + &state->upfront_shutdown_script[REMOTE])) peer_failed(state->pps, &state->channel_id, "Parsing accept_channel with option_upfront_shutdown_script %s", tal_hex(msg, msg)); @@ -866,7 +867,7 @@ static u8 *funder_channel_complete(struct state *state) state->funding_txout, state->feerate_per_kw, state->localconf.channel_reserve, - state->remote_upfront_shutdown_script); + state->upfront_shutdown_script[REMOTE]); } /*~ The peer sent us an `open_channel`, that means we're the fundee. */ @@ -878,15 +879,11 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) struct bitcoin_signature theirsig, sig; struct bitcoin_tx *local_commit, *remote_commit; struct bitcoin_blkid chain_hash; - u8 *msg, *our_upfront_shutdown_script; + u8 *msg; const u8 *wscript; u8 channel_flags; char* err_reason; - /* Default is no shutdown_scriptpubkey: free any leftover one. */ - state->remote_upfront_shutdown_script - = tal_free(state->remote_upfront_shutdown_script); - /* BOLT #2: * * The receiving node MUST fail the channel if: @@ -916,7 +913,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) &theirs.htlc, &state->first_per_commitment_point[REMOTE], &channel_flags, - &state->remote_upfront_shutdown_script)) + &state->upfront_shutdown_script[REMOTE])) peer_failed(state->pps, &state->channel_id, "Parsing open_channel with option_upfront_shutdown_script %s", tal_hex(tmpctx, open_channel_msg)); @@ -1060,12 +1057,14 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) state->remoteconf.to_self_delay, state->remoteconf.max_accepted_htlcs, channel_flags, - state->remote_upfront_shutdown_script); + state->upfront_shutdown_script[REMOTE]); wire_sync_write(REQ_FD, take(msg)); msg = wire_sync_read(tmpctx, REQ_FD); - if (!fromwire_opening_got_offer_reply(NULL, msg, &err_reason, - &our_upfront_shutdown_script)) + /* We don't allocate off tmpctx, because that's freed inside + * opening_negotiate_msg */ + if (!fromwire_opening_got_offer_reply(state, msg, &err_reason, + &state->upfront_shutdown_script[LOCAL])) master_badmsg(WIRE_OPENING_GOT_OFFER_REPLY, msg); /* If they give us a reason to reject, do so. */ @@ -1073,11 +1072,12 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) u8 *errmsg = towire_errorfmt(NULL, &state->channel_id, "%s", err_reason); sync_crypto_write(state->pps, take(errmsg)); + tal_free(err_reason); return NULL; } - if (!our_upfront_shutdown_script) - our_upfront_shutdown_script = dev_upfront_shutdown_script(state); + if (!state->upfront_shutdown_script[LOCAL]) + state->upfront_shutdown_script[LOCAL] = dev_upfront_shutdown_script(state); /* OK, we accept! */ msg = towire_accept_channel_option_upfront_shutdown_script(NULL, &state->channel_id, @@ -1094,7 +1094,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) &state->our_points.delayed_payment, &state->our_points.htlc, &state->first_per_commitment_point[LOCAL], - our_upfront_shutdown_script); + state->upfront_shutdown_script[LOCAL]); sync_crypto_write(state->pps, take(msg)); @@ -1262,8 +1262,8 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) state->feerate_per_kw, msg, state->localconf.channel_reserve, - our_upfront_shutdown_script, - state->remote_upfront_shutdown_script); + state->upfront_shutdown_script[LOCAL], + state->upfront_shutdown_script[REMOTE]); } /*~ Standard "peer sent a message, handle it" demuxer. Though it really only @@ -1359,20 +1359,19 @@ static u8 *handle_master_in(struct state *state) { u8 *msg = wire_sync_read(tmpctx, REQ_FD); enum opening_wire_type t = fromwire_peektype(msg); - u8 channel_flags, *upfront_shutdown_script; + u8 channel_flags; struct bitcoin_txid funding_txid; u16 funding_txout; switch (t) { case WIRE_OPENING_FUNDER_START: - if (!fromwire_opening_funder_start(tmpctx, msg, &state->funding, + if (!fromwire_opening_funder_start(state, msg, &state->funding, &state->push_msat, - &upfront_shutdown_script, + &state->upfront_shutdown_script[LOCAL], &state->feerate_per_kw, &channel_flags)) master_badmsg(WIRE_OPENING_FUNDER_START, msg); - msg = funder_channel_start(state, upfront_shutdown_script, - channel_flags); + msg = funder_channel_start(state, channel_flags); /* We want to keep openingd alive, since we're not done yet */ if (msg) @@ -1478,8 +1477,10 @@ int main(int argc, char *argv[]) memset(&state->channel_id, 0, sizeof(state->channel_id)); state->channel = NULL; - /*~ We set this to NULL, meaning no requirements on shutdown */ - state->remote_upfront_shutdown_script = NULL; + /*~ We set these to NULL, meaning no requirements on shutdown */ + state->upfront_shutdown_script[LOCAL] + = state->upfront_shutdown_script[REMOTE] + = NULL; /*~ We need an initial per-commitment point whether we're funding or * they are, and lightningd has reserved a unique dbid for us already,