From 3b0c246cc9cd32dacf8886373f0fb1a88073de44 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 14:27:11 +0930 Subject: [PATCH 01/15] bitcoin/psbt: more const pointers. Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 8 ++++---- bitcoin/psbt.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index 71f267e1da23..0bd1c23a5553 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -359,8 +359,8 @@ void psbt_elements_normalize_fees(struct wally_psbt *psbt) } } -bool psbt_has_input(struct wally_psbt *psbt, - struct bitcoin_txid *txid, +bool psbt_has_input(const struct wally_psbt *psbt, + const struct bitcoin_txid *txid, u32 outnum) { for (size_t i = 0; i < psbt->num_inputs; i++) { @@ -388,7 +388,7 @@ bool psbt_input_set_redeemscript(struct wally_psbt *psbt, size_t in, return wally_err == WALLY_OK; } -struct amount_sat psbt_input_get_amount(struct wally_psbt *psbt, +struct amount_sat psbt_input_get_amount(const struct wally_psbt *psbt, size_t in) { struct amount_sat val; @@ -408,7 +408,7 @@ struct amount_sat psbt_input_get_amount(struct wally_psbt *psbt, return val; } -struct amount_sat psbt_output_get_amount(struct wally_psbt *psbt, +struct amount_sat psbt_output_get_amount(const struct wally_psbt *psbt, size_t out) { struct amount_asset asset; diff --git a/bitcoin/psbt.h b/bitcoin/psbt.h index 312dcf22e957..7479821426a8 100644 --- a/bitcoin/psbt.h +++ b/bitcoin/psbt.h @@ -201,7 +201,7 @@ void psbt_output_add_unknown(struct wally_psbt_output *out, * @psbt - psbt * @in - index of input whose value you're returning * */ -struct amount_sat psbt_input_get_amount(struct wally_psbt *psbt, +struct amount_sat psbt_input_get_amount(const struct wally_psbt *psbt, size_t in); /* psbt_output_get_amount - Returns the value of this output @@ -209,7 +209,7 @@ struct amount_sat psbt_input_get_amount(struct wally_psbt *psbt, * @psbt - psbt * @out -index of output whose value you're returning */ -struct amount_sat psbt_output_get_amount(struct wally_psbt *psbt, +struct amount_sat psbt_output_get_amount(const struct wally_psbt *psbt, size_t out); /* psbt_has_input - Is this input present on this psbt @@ -218,8 +218,8 @@ struct amount_sat psbt_output_get_amount(struct wally_psbt *psbt, * @txid - txid of input * @outnum - output index of input */ -bool psbt_has_input(struct wally_psbt *psbt, - struct bitcoin_txid *txid, +bool psbt_has_input(const struct wally_psbt *psbt, + const struct bitcoin_txid *txid, u32 outnum); struct wally_psbt *psbt_from_b64(const tal_t *ctx, From 32f035d6c945f5eee5f70fbd1b70b3ac3ea885ea Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 19:54:03 +0930 Subject: [PATCH 02/15] bitcoin/psbt: attach destructors to wally allocations to avoid leaks. This covers the obvious ones, but the later patches fix this more seriously. Signed-off-by: Rusty Russell Changelog-Fixed: Some memory leaks in transaction and PSBT manipulate closed. --- bitcoin/psbt.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index 0bd1c23a5553..ddb5fc632a5e 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -49,6 +49,7 @@ struct wally_psbt *create_psbt(const tal_t *ctx, size_t num_inputs, size_t num_o wally_err = wally_psbt_set_global_tx(psbt, wtx); assert(wally_err == WALLY_OK); + wally_tx_free(wtx); return psbt; } @@ -252,7 +253,7 @@ bool psbt_input_set_signature(struct wally_psbt *psbt, size_t in, void psbt_input_set_wit_utxo(struct wally_psbt *psbt, size_t in, const u8 *scriptPubkey, struct amount_sat amt) { - struct wally_tx_output tx_out; + struct wally_tx_output *tx_out; int wally_err; assert(in < psbt->num_inputs); @@ -265,7 +266,7 @@ void psbt_input_set_wit_utxo(struct wally_psbt *psbt, size_t in, sizeof(value)); assert(wally_err == WALLY_OK); wally_err = - wally_tx_elements_output_init(scriptPubkey, + wally_tx_elements_output_init_alloc(scriptPubkey, tal_bytelen(scriptPubkey), chainparams->fee_asset_tag, ELEMENTS_ASSET_LEN, @@ -274,12 +275,13 @@ void psbt_input_set_wit_utxo(struct wally_psbt *psbt, size_t in, NULL, 0, &tx_out); } else - wally_err = wally_tx_output_init(amt.satoshis, /* Raw: type conv */ + wally_err = wally_tx_output_init_alloc(amt.satoshis, /* Raw: type conv */ scriptPubkey, tal_bytelen(scriptPubkey), &tx_out); assert(wally_err == WALLY_OK); - wally_err = wally_psbt_input_set_witness_utxo(&psbt->inputs[in], &tx_out); + wally_err = wally_psbt_input_set_witness_utxo(&psbt->inputs[in], tx_out); + wally_tx_output_free(tx_out); assert(wally_err == WALLY_OK); } @@ -516,6 +518,12 @@ void psbt_output_add_unknown(struct wally_psbt_output *out, abort(); } +/* Use the destructor to free the wally_tx */ +static void wally_tx_destroy(struct wally_tx *wtx) +{ + wally_tx_free(wtx); +} + struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place) { struct wally_psbt *tmppsbt; @@ -527,6 +535,7 @@ struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place) if (!finalize_in_place) { if (wally_psbt_clone_alloc(psbt, 0, &tmppsbt) != WALLY_OK) return NULL; + tal_add_destructor(tmppsbt, psbt_destroy); } else tmppsbt = cast_const(struct wally_psbt *, psbt); @@ -576,6 +585,7 @@ struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place) if (psbt_is_finalized(tmppsbt) && wally_psbt_extract(tmppsbt, &wtx) == WALLY_OK) { + tal_add_destructor(wtx, wally_tx_destroy); if (!finalize_in_place) wally_psbt_free(tmppsbt); return wtx; From ddcadaeec0ce5d1bbae579eba728a55340b89bba Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 19:54:26 +0930 Subject: [PATCH 03/15] bitcoin/tx: trivial cleanups. Signed-off-by: Rusty Russell --- bitcoin/tx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 714a3684f5e6..bbdf306a2b3e 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -346,7 +346,7 @@ void bitcoin_tx_input_set_witness(struct bitcoin_tx *tx, int innum, if (stack) wally_tx_witness_stack_free(stack); if (taken(witness)) - tal_free(witness); + tal_free(witness); } void bitcoin_tx_input_set_script(struct bitcoin_tx *tx, int innum, u8 *script) @@ -478,12 +478,10 @@ struct bitcoin_tx *bitcoin_tx(const tal_t *ctx, if (chainparams->is_elements) output_count += 1; - wally_tx_init_alloc(WALLY_TX_VERSION_2, 0, input_count, output_count, + wally_tx_init_alloc(WALLY_TX_VERSION_2, nlocktime, input_count, output_count, &tx->wtx); tal_add_destructor(tx, bitcoin_tx_destroy); - tx->wtx->locktime = nlocktime; - tx->wtx->version = 2; tx->chainparams = chainparams; tx->psbt = new_psbt(tx, tx->wtx); From 46ea7cb0c5676d2b4b2baa89e7e87a30eeb4546e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 19:54:26 +0930 Subject: [PATCH 04/15] bitcoin/psbt: psbt_txid needs a tal ctx. It returns a wally_tx; it's an anti-pattern not to hand in a tal context. Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 5 +++-- bitcoin/psbt.h | 4 +++- openingd/dualopend.c | 3 +-- plugins/spender/multifundchannel.c | 2 +- plugins/txprepare.c | 4 ++-- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index ddb5fc632a5e..2402f14df6fe 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -706,7 +706,8 @@ struct wally_psbt *fromwire_wally_psbt(const tal_t *ctx, } /* This only works on a non-final psbt because we're ALL SEGWIT! */ -void psbt_txid(const struct wally_psbt *psbt, struct bitcoin_txid *txid, +void psbt_txid(const tal_t *ctx, + const struct wally_psbt *psbt, struct bitcoin_txid *txid, struct wally_tx **wtx) { struct wally_tx *tx; @@ -732,7 +733,7 @@ void psbt_txid(const struct wally_psbt *psbt, struct bitcoin_txid *txid, wally_txid(tx, txid); if (wtx) - *wtx = tx; + *wtx = tal_steal(ctx, tx); else wally_tx_free(tx); } diff --git a/bitcoin/psbt.h b/bitcoin/psbt.h index 7479821426a8..b74a18203326 100644 --- a/bitcoin/psbt.h +++ b/bitcoin/psbt.h @@ -62,11 +62,13 @@ bool psbt_is_finalized(const struct wally_psbt *psbt); /** * psbt_txid - get the txid of the psbt (what it would be after finalization) + * @ctx: the context to allocate wtx off, if *@wtx isn't NULL. * @psbt: the psbt. * @txid: the transaction id (output) * @wtx: if non-NULL, returns a copy of the transaction (caller must wally_tx_free). */ -void psbt_txid(const struct wally_psbt *psbt, struct bitcoin_txid *txid, +void psbt_txid(const tal_t *ctx, + const struct wally_psbt *psbt, struct bitcoin_txid *txid, struct wally_tx **wtx); /* psbt_elements_normalize_fees - Figure out the fee output for a PSET diff --git a/openingd/dualopend.c b/openingd/dualopend.c index e563db2d65bc..7ca4bb7121cd 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -1036,8 +1036,7 @@ static u8 *accepter_start(struct state *state, const u8 *oc2_msg) return NULL; /* Find the funding transaction txid */ - struct wally_tx *funding_tx; - psbt_txid(psbt, &state->funding_txid, &funding_tx); + psbt_txid(NULL, psbt, &state->funding_txid, NULL); wscript = bitcoin_redeem_2of2(state, &state->our_funding_pubkey, diff --git a/plugins/spender/multifundchannel.c b/plugins/spender/multifundchannel.c index 20b0520511cd..37baa66138d5 100644 --- a/plugins/spender/multifundchannel.c +++ b/plugins/spender/multifundchannel.c @@ -1392,7 +1392,7 @@ perform_funding_tx_finalize(struct multifundchannel_command *mfc) /* Generate the TXID. */ mfc->txid = tal(mfc, struct bitcoin_txid); - psbt_txid(mfc->psbt, mfc->txid, NULL); + psbt_txid(NULL, mfc->psbt, mfc->txid, NULL); plugin_log(mfc->cmd->plugin, LOG_DBG, "mfc %"PRIu64": funding tx %s: %s", diff --git a/plugins/txprepare.c b/plugins/txprepare.c index bd579c8274fc..be7414e1694f 100644 --- a/plugins/txprepare.c +++ b/plugins/txprepare.c @@ -150,7 +150,7 @@ static struct command_result *signpsbt_done(struct command *cmd, tal_free(utx->tx); /* The txid from the final should match our expectation. */ - psbt_txid(utx->psbt, &txid, &utx->tx); + psbt_txid(utx, utx->psbt, &txid, &utx->tx); if (!bitcoin_txid_eq(&txid, &utx->txid)) { return command_fail(cmd, LIGHTNINGD, "Signed tx changed txid? Had '%s' now '%s'", @@ -197,7 +197,7 @@ static struct command_result *finish_txprepare(struct command *cmd, utx = tal(NULL, struct unreleased_tx); utx->psbt = tal_steal(utx, txp->psbt); - psbt_txid(txp->psbt, &utx->txid, &utx->tx); + psbt_txid(utx, txp->psbt, &utx->txid, &utx->tx); /* If this is a withdraw, we sign and send immediately. */ if (txp->is_withdraw) { From 596298a2e105229332f5243d17b63cd9eca0493e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 19:54:26 +0930 Subject: [PATCH 05/15] bitcoin/psbt: wallt_tx_output needs a tal ctx. Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 4 ++-- bitcoin/tx.c | 7 ++++--- bitcoin/tx.h | 3 ++- plugins/txprepare.c | 3 ++- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index 2402f14df6fe..bc49222cb0ec 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -178,7 +178,7 @@ struct wally_psbt_output *psbt_append_output(struct wally_psbt *psbt, struct amount_sat amount) { struct wally_psbt_output *out; - struct wally_tx_output *tx_out = wally_tx_output(script, amount); + struct wally_tx_output *tx_out = wally_tx_output(NULL, script, amount); out = psbt_add_output(psbt, tx_out, psbt->tx->num_outputs); wally_tx_output_free(tx_out); @@ -190,7 +190,7 @@ struct wally_psbt_output *psbt_insert_output(struct wally_psbt *psbt, size_t insert_at) { struct wally_psbt_output *out; - struct wally_tx_output *tx_out = wally_tx_output(script, amount); + struct wally_tx_output *tx_out = wally_tx_output(NULL, script, amount); out = psbt_add_output(psbt, tx_out, insert_at); wally_tx_output_free(tx_out); diff --git a/bitcoin/tx.c b/bitcoin/tx.c index bbdf306a2b3e..9a8d09b997eb 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -26,7 +26,8 @@ struct bitcoin_tx_output *new_tx_output(const tal_t *ctx, return output; } -struct wally_tx_output *wally_tx_output(const u8 *script, +struct wally_tx_output *wally_tx_output(const tal_t *ctx, + const u8 *script, struct amount_sat amount) { u64 satoshis = amount.satoshis; /* Raw: wally API */ @@ -53,7 +54,7 @@ struct wally_tx_output *wally_tx_output(const u8 *script, if (ret != WALLY_OK) return NULL; } - return output; + return tal_steal(ctx, output); } int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script, @@ -69,7 +70,7 @@ int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script, assert(tx->wtx != NULL); assert(chainparams); - output = wally_tx_output(script, amount); + output = wally_tx_output(NULL, script, amount); assert(output); ret = wally_tx_add_output(tx->wtx, output); assert(ret == WALLY_OK); diff --git a/bitcoin/tx.h b/bitcoin/tx.h index c2e1bc299ec8..2c2a876364e1 100644 --- a/bitcoin/tx.h +++ b/bitcoin/tx.h @@ -83,7 +83,8 @@ struct bitcoin_tx *pull_bitcoin_tx(const tal_t *ctx, /* Helper to create a wally_tx_output: make sure to wally_tx_output_free! * Returns NULL if amount is extreme (wally doesn't like). */ -struct wally_tx_output *wally_tx_output(const u8 *script, +struct wally_tx_output *wally_tx_output(const tal_t *ctx, + const u8 *script, struct amount_sat amount); /* Add one output to tx. */ diff --git a/plugins/txprepare.c b/plugins/txprepare.c index be7414e1694f..a37610363455 100644 --- a/plugins/txprepare.c +++ b/plugins/txprepare.c @@ -178,7 +178,7 @@ static struct command_result *finish_txprepare(struct command *cmd, for (size_t i = 0; i < tal_count(txp->outputs); i++) { struct wally_tx_output *out; - out = wally_tx_output(txp->outputs[i].script, + out = wally_tx_output(NULL, txp->outputs[i].script, txp->outputs[i].amount); if (!out) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, @@ -189,6 +189,7 @@ static struct command_result *finish_txprepare(struct command *cmd, struct amount_sat, &txp->outputs[i].amount)); psbt_add_output(txp->psbt, out, i); + wally_tx_output_free(out); } /* If this is elements, we should normalize From 4b7199c579e3d564811641c9e9ce3020816e0444 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Sep 2020 19:54:26 +0930 Subject: [PATCH 06/15] bitcoin/psbt: psbt_finalize needs a tal ctx. Since it returns a wally_tx. Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 4 +++- bitcoin/psbt.h | 3 ++- bitcoin/tx.c | 2 +- lightningd/dual_open_control.c | 2 +- wallet/walletrpc.c | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index bc49222cb0ec..7a45dedf52aa 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -524,7 +524,8 @@ static void wally_tx_destroy(struct wally_tx *wtx) wally_tx_free(wtx); } -struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place) +struct wally_tx *psbt_finalize(const tal_t *ctx, + struct wally_psbt *psbt, bool finalize_in_place) { struct wally_psbt *tmppsbt; struct wally_tx *wtx; @@ -585,6 +586,7 @@ struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place) if (psbt_is_finalized(tmppsbt) && wally_psbt_extract(tmppsbt, &wtx) == WALLY_OK) { + tal_steal(ctx, wtx); tal_add_destructor(wtx, wally_tx_destroy); if (!finalize_in_place) wally_psbt_free(tmppsbt); diff --git a/bitcoin/psbt.h b/bitcoin/psbt.h index b74a18203326..c0d605208706 100644 --- a/bitcoin/psbt.h +++ b/bitcoin/psbt.h @@ -79,7 +79,8 @@ void psbt_txid(const tal_t *ctx, */ void psbt_elements_normalize_fees(struct wally_psbt *psbt); -struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place); +struct wally_tx *psbt_finalize(const tal_t *ctx, + struct wally_psbt *psbt, bool finalize_in_place); /* psbt_make_key - Create a new, proprietary c-lightning key * diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 9a8d09b997eb..e8b513def049 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -502,7 +502,7 @@ struct bitcoin_tx *bitcoin_tx_with_psbt(const tal_t *ctx, struct wally_psbt *psb psbt->tx->num_outputs, psbt->tx->locktime); wally_tx_free(tx->wtx); - tx->wtx = psbt_finalize(psbt, false); + tx->wtx = psbt_finalize(tx, psbt, false); if (!tx->wtx && wally_tx_clone_alloc(psbt->tx, 0, &tx->wtx) != WALLY_OK) return NULL; diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 2cbfd38a3e9b..b9e3690e4b26 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -590,7 +590,7 @@ openchannel2_sign_hook_cb(struct openchannel2_psbt_payload *payload STEALS) tal_steal(tmpctx, payload); /* Finalize it, if not already. It shouldn't work entirely */ - psbt_finalize(payload->psbt, true); + psbt_finalize(tmpctx, payload->psbt, true); if (!psbt_side_finalized(payload->ld->log, payload->psbt, REMOTE)) fatal("Plugin must return a 'psbt' with signatures for their inputs" diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index 26f8073d3bcd..d2baa7fc6baa 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -812,7 +812,7 @@ static struct command_result *json_sendpsbt(struct command *cmd, sending = tal(cmd, struct sending_psbt); sending->cmd = cmd; - sending->wtx = tal_steal(sending, psbt_finalize(psbt, true)); + sending->wtx = psbt_finalize(sending, psbt, true); if (!sending->wtx) return command_fail(cmd, LIGHTNINGD, "PSBT not finalizeable %s", From ea2681e809d06228d65ada57b7e9bc85aa00575a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 22 Sep 2020 19:22:37 +0930 Subject: [PATCH 07/15] bitcoin/psbt: psbt_input_add_unknown/psbt_output_add_unknown needs a tal ctx. Since it allocates something, it needs a context (used in the next patch!). Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 8 ++++---- bitcoin/psbt.h | 8 ++++++-- common/psbt_open.c | 10 ++++++---- common/psbt_open.h | 10 +++++++--- common/test/exp-run-psbt_diff.c | 8 ++++---- lightningd/dual_open_control.c | 4 ++-- openingd/dualopend.c | 4 ++-- 7 files changed, 31 insertions(+), 21 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index 7a45dedf52aa..40ad8f63dd0d 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -464,7 +464,8 @@ u8 *psbt_make_key(const tal_t *ctx, u8 key_subtype, const u8 *key_data) return key; } -void psbt_input_add_unknown(struct wally_psbt_input *in, +void psbt_input_add_unknown(const tal_t *ctx, + struct wally_psbt_input *in, const u8 *key, const void *value, size_t value_len) @@ -506,7 +507,8 @@ void *psbt_get_lightning(const struct wally_map *map, } -void psbt_output_add_unknown(struct wally_psbt_output *out, +void psbt_output_add_unknown(const tal_t *ctx, + struct wally_psbt_output *out, const u8 *key, const void *value, size_t value_len) @@ -626,8 +628,6 @@ char *psbt_to_b64(const tal_t *ctx, const struct wally_psbt *psbt) wally_free_string(serialized_psbt); return ret_val; } - -/* Do not remove this line, it is magic */ REGISTER_TYPE_TO_STRING(wally_psbt, psbt_to_b64); const u8 *psbt_get_bytes(const tal_t *ctx, const struct wally_psbt *psbt, diff --git a/bitcoin/psbt.h b/bitcoin/psbt.h index c0d605208706..da4819065d84 100644 --- a/bitcoin/psbt.h +++ b/bitcoin/psbt.h @@ -157,12 +157,14 @@ void psbt_elements_input_init_witness(struct wally_psbt *psbt, size_t in, bool psbt_input_set_redeemscript(struct wally_psbt *psbt, size_t in, const u8 *redeemscript); /* psbt_input_add_unknown - Add the given Key-Value to the psbt's input keymap + * @ctx - tal context for allocations * @in - psbt input to add key-value to * @key - key for key-value pair * @value - value to add * @value_len - length of {@value} */ -void psbt_input_add_unknown(struct wally_psbt_input *in, +void psbt_input_add_unknown(const tal_t *ctx, + struct wally_psbt_input *in, const u8 *key, const void *value, size_t value_len); @@ -190,12 +192,14 @@ void *psbt_get_lightning(const struct wally_map *map, /* psbt_output_add_unknown - Add the given Key-Value to the psbt's output keymap * + * @ctx - tal context for allocations * @out - psbt output to add key-value to * @key - key for key-value pair * @value - value to add * @value_len - length of {@value} */ -void psbt_output_add_unknown(struct wally_psbt_output *out, +void psbt_output_add_unknown(const tal_t *ctx, + struct wally_psbt_output *out, const u8 *key, const void *value, size_t value_len); diff --git a/common/psbt_open.c b/common/psbt_open.c index c84bc7de6a1b..b505a34df7ed 100644 --- a/common/psbt_open.c +++ b/common/psbt_open.c @@ -392,22 +392,24 @@ u8 *psbt_changeset_get_next(const tal_t *ctx, struct channel_id *cid, return NULL; } -void psbt_input_add_serial_id(struct wally_psbt_input *input, +void psbt_input_add_serial_id(const tal_t *ctx, + struct wally_psbt_input *input, u16 serial_id) { u8 *key = psbt_make_key(tmpctx, PSBT_TYPE_SERIAL_ID, NULL); beint16_t bev = cpu_to_be16(serial_id); - psbt_input_add_unknown(input, key, &bev, sizeof(bev)); + psbt_input_add_unknown(ctx, input, key, &bev, sizeof(bev)); } -void psbt_output_add_serial_id(struct wally_psbt_output *output, +void psbt_output_add_serial_id(const tal_t *ctx, + struct wally_psbt_output *output, u16 serial_id) { u8 *key = psbt_make_key(tmpctx, PSBT_TYPE_SERIAL_ID, NULL); beint16_t bev = cpu_to_be16(serial_id); - psbt_output_add_unknown(output, key, &bev, sizeof(bev)); + psbt_output_add_unknown(ctx, output, key, &bev, sizeof(bev)); } int psbt_find_serial_input(struct wally_psbt *psbt, u16 serial_id) diff --git a/common/psbt_open.h b/common/psbt_open.h index 7c1f96320593..b4be8d093f64 100644 --- a/common/psbt_open.h +++ b/common/psbt_open.h @@ -85,17 +85,21 @@ u8 *psbt_changeset_get_next(const tal_t *ctx, struct channel_id *cid, /* psbt_input_add_serial_id - Adds a serial id to given input * + * @ctx - tal context for allocations * @input - to add serial_id to * @serial_id - to add */ -void psbt_input_add_serial_id(struct wally_psbt_input *input, - u16 serial_id); +void psbt_input_add_serial_id(const tal_t *ctx, + struct wally_psbt_input *input, + u16 serial_id); /* psbt_output_add_serial_id - Adds a serial id to given output * + * @ctx - tal context for allocations * @output - to add serial_id to * @serial_id - to add */ -void psbt_output_add_serial_id(struct wally_psbt_output *output, +void psbt_output_add_serial_id(const tal_t *ctx, + struct wally_psbt_output *output, u16 serial_id); /* psbt_sort_by_serial_id - Sorts the inputs + outputs by serial_id diff --git a/common/test/exp-run-psbt_diff.c b/common/test/exp-run-psbt_diff.c index 9d077c09a16d..ae2be627b733 100644 --- a/common/test/exp-run-psbt_diff.c +++ b/common/test/exp-run-psbt_diff.c @@ -114,7 +114,7 @@ static void add_in_out_with_serial(struct wally_psbt *psbt, NULL, NULL, NULL); if (!in) abort(); - psbt_input_add_serial_id(in, serial_id); + psbt_input_add_serial_id(psbt, in, serial_id); script = tal_arr(tmpctx, u8, 20); memset(script, default_value, 20); @@ -122,7 +122,7 @@ static void add_in_out_with_serial(struct wally_psbt *psbt, out = psbt_append_output(psbt, script, sat); if (!out) abort(); - psbt_output_add_serial_id(out, serial_id); + psbt_output_add_serial_id(psbt, out, serial_id); } int main(int argc, const char *argv[]) @@ -182,8 +182,8 @@ int main(int argc, const char *argv[]) /* Add some extra unknown info to a PSBT */ u8 *key = psbt_make_key(tmpctx, 0x05, NULL); char *val = tal_fmt(tmpctx, "hello"); - psbt_input_add_unknown(&end->inputs[1], key, val, tal_bytelen(val)); - psbt_input_add_unknown(&start->inputs[1], key, val, tal_bytelen(val)); + psbt_input_add_unknown(end, &end->inputs[1], key, val, tal_bytelen(val)); + psbt_input_add_unknown(start, &start->inputs[1], key, val, tal_bytelen(val)); /* Swap locations */ struct wally_map_item tmp; diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index b9e3690e4b26..5dd1e5bda68c 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -431,7 +431,7 @@ static void psbt_add_serials(struct wally_psbt *psbt, enum side opener) psbt_find_serial_input(psbt, serial_id) != -1) { /* keep going; */ } - psbt_input_add_serial_id(&psbt->inputs[i], serial_id); + psbt_input_add_serial_id(psbt, &psbt->inputs[i], serial_id); } for (size_t i = 0; i < psbt->num_outputs; i++) { /* Skip ones that already have a serial id */ @@ -442,7 +442,7 @@ static void psbt_add_serials(struct wally_psbt *psbt, enum side opener) psbt_find_serial_output(psbt, serial_id) != -1) { /* keep going; */ } - psbt_output_add_serial_id(&psbt->outputs[i], serial_id); + psbt_output_add_serial_id(psbt, &psbt->outputs[i], serial_id); } } diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 7ca4bb7121cd..0810a73c6682 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -686,7 +686,7 @@ static bool run_tx_interactive(struct state *state, struct wally_psbt **orig_psb psbt_elements_input_set_asset(psbt, outnum, &asset); } - psbt_input_add_serial_id(in, serial_id); + psbt_input_add_serial_id(psbt, in, serial_id); /* FIXME: what's in the tlv? */ break; @@ -747,7 +747,7 @@ static bool run_tx_interactive(struct state *state, struct wally_psbt **orig_psb "Duplicate serial_id rcvd. %u", serial_id); amt = amount_sat(value); out = psbt_append_output(psbt, scriptpubkey, amt); - psbt_output_add_serial_id(out, serial_id); + psbt_output_add_serial_id(psbt, out, serial_id); break; } case WIRE_TX_REMOVE_OUTPUT: { From 88e3c149e1cb9d9c3027d2920aa6b63c6358aa09 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 22 Sep 2020 19:23:37 +0930 Subject: [PATCH 08/15] common: add tal_gather_wally() function to reparent libwally objs. This lets us reduce leaks, and ease their detection. Signed-off-by: Rusty Russell --- common/setup.c | 2 -- common/utils.c | 10 ++++++++++ common/utils.h | 6 ++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/common/setup.c b/common/setup.c index fa0462e9510f..f0d5c0d5fba8 100644 --- a/common/setup.c +++ b/common/setup.c @@ -5,8 +5,6 @@ #include #include -static const tal_t *wally_tal_ctx; - static void *wally_tal(size_t size) { return tal_arr_label(wally_tal_ctx, u8, size, "wally_notleak"); diff --git a/common/utils.c b/common/utils.c index 37c4457fc488..a67321873ad5 100644 --- a/common/utils.c +++ b/common/utils.c @@ -5,6 +5,7 @@ #include #include +const tal_t *wally_tal_ctx; secp256k1_context *secp256k1_ctx; const tal_t *tmpctx; @@ -15,6 +16,15 @@ bool is_elements(const struct chainparams *chainparams) return chainparams->is_elements; } +/* Steal any wally allocations onto this context. */ +void tal_gather_wally(const tal_t *ctx) +{ + tal_t *p; + assert(tal_first(wally_tal_ctx)); + while ((p = tal_first(wally_tal_ctx)) != NULL) + tal_steal(ctx, p); +} + #if DEVELOPER /* If you've got a softref, we assume no reallocs. */ static void dont_move_softref(tal_t *ctx, enum tal_notify_type ntype, void *info) diff --git a/common/utils.h b/common/utils.h index 925228de70f3..a6b273405909 100644 --- a/common/utils.h +++ b/common/utils.h @@ -84,6 +84,9 @@ void setup_tmpctx(void); /* Free any children of tmpctx. */ void clean_tmpctx(void); +/* Steal any wally allocations onto this context. */ +void tal_gather_wally(const tal_t *ctx); + /* Define sha256_eq. */ STRUCTEQ_DEF(sha256, 0, u); @@ -106,4 +109,7 @@ STRUCTEQ_DEF(ripemd160, 0, u); #define IFDEV(dev, nondev) (nondev) #endif +/* Context which all wally allocations use (see common/setup.c) */ +extern const tal_t *wally_tal_ctx; + #endif /* LIGHTNING_COMMON_UTILS_H */ From 40ee7fc31ae3d6f42b798ed9e6ad1da0325ff1d8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:02 +0930 Subject: [PATCH 09/15] bitcoin: use tal_gather_wally() so we don't leave unattached allocations. Signed-off-by: Rusty Russell --- bitcoin/psbt.c | 50 ++++++++++++++++++++++++++++++++++++---------- bitcoin/tx.c | 26 +++++++++++++++++++----- bitcoin/tx_parts.c | 19 ++++++++++++++---- hsmd/hsmd.c | 1 + 4 files changed, 76 insertions(+), 20 deletions(-) diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index 40ad8f63dd0d..3aef70148738 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -32,8 +32,12 @@ static struct wally_psbt *init_psbt(const tal_t *ctx, size_t num_inputs, size_t else wally_err = wally_psbt_init_alloc(0, num_inputs, num_outputs, 0, &psbt); assert(wally_err == WALLY_OK); + tal_steal(ctx, psbt); + /* If both of these are zero, no sub-allocations were done */ + if (num_inputs || num_outputs) + tal_gather_wally(psbt); tal_add_destructor(psbt, psbt_destroy); - return tal_steal(ctx, psbt); + return psbt; } struct wally_psbt *create_psbt(const tal_t *ctx, size_t num_inputs, size_t num_outputs, u32 locktime) @@ -44,12 +48,14 @@ struct wally_psbt *create_psbt(const tal_t *ctx, size_t num_inputs, size_t num_o if (wally_tx_init_alloc(WALLY_TX_VERSION_2, locktime, num_inputs, num_outputs, &wtx) != WALLY_OK) abort(); + tal_gather_wally(tal_steal(NULL, wtx)); psbt = init_psbt(ctx, num_inputs, num_outputs); wally_err = wally_psbt_set_global_tx(psbt, wtx); assert(wally_err == WALLY_OK); wally_tx_free(wtx); + tal_gather_wally(psbt); return psbt; } @@ -84,7 +90,8 @@ struct wally_psbt *new_psbt(const tal_t *ctx, const struct wally_tx *wtx) } } - return tal_steal(ctx, psbt); + tal_gather_wally(tal_steal(ctx, psbt)); + return psbt; } bool psbt_is_finalized(const struct wally_psbt *psbt) @@ -104,6 +111,7 @@ struct wally_psbt_input *psbt_add_input(struct wally_psbt *psbt, wally_err = wally_psbt_add_input_at(psbt, insert_at, flags, input); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); return &psbt->inputs[insert_at]; } @@ -141,6 +149,7 @@ struct wally_psbt_input *psbt_append_input(struct wally_psbt *psbt, wally_err = wally_psbt_add_input_at(psbt, input_num, flags, tx_in); assert(wally_err == WALLY_OK); wally_tx_input_free(tx_in); + tal_gather_wally(psbt); if (input_wscript) { /* Add the prev output's data into the PSBT struct */ @@ -152,6 +161,7 @@ struct wally_psbt_input *psbt_append_input(struct wally_psbt *psbt, redeemscript, tal_bytelen(redeemscript)); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); } return &psbt->inputs[input_num]; @@ -170,6 +180,7 @@ struct wally_psbt_output *psbt_add_output(struct wally_psbt *psbt, { int wally_err = wally_psbt_add_output_at(psbt, insert_at, 0, output); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); return &psbt->outputs[insert_at]; } @@ -231,6 +242,7 @@ void psbt_input_add_pubkey(struct wally_psbt *psbt, size_t in, fingerprint, sizeof(fingerprint), empty_path, ARRAY_SIZE(empty_path)); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); } bool psbt_input_set_signature(struct wally_psbt *psbt, size_t in, @@ -244,10 +256,13 @@ bool psbt_input_set_signature(struct wally_psbt *psbt, size_t in, /* we serialize the compressed version of the key, wally likes this */ pubkey_to_der(pk_der, pubkey); wally_psbt_input_set_sighash(&psbt->inputs[in], sig->sighash_type); - return wally_psbt_input_add_signature(&psbt->inputs[in], - pk_der, sizeof(pk_der), - sig->s.data, - sizeof(sig->s.data)) == WALLY_OK; + if (wally_psbt_input_add_signature(&psbt->inputs[in], + pk_der, sizeof(pk_der), + sig->s.data, + sizeof(sig->s.data)) != WALLY_OK) + return false; + tal_gather_wally(psbt); + return true; } void psbt_input_set_wit_utxo(struct wally_psbt *psbt, size_t in, @@ -283,6 +298,7 @@ void psbt_input_set_wit_utxo(struct wally_psbt *psbt, size_t in, wally_err = wally_psbt_input_set_witness_utxo(&psbt->inputs[in], tx_out); wally_tx_output_free(tx_out); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); } void psbt_input_set_witscript(struct wally_psbt *psbt, size_t in, const u8 *wscript) @@ -293,6 +309,7 @@ void psbt_input_set_witscript(struct wally_psbt *psbt, size_t in, const u8 *wscr wscript, tal_bytelen(wscript)); assert(wally_err == WALLY_OK); + tal_gather_wally(psbt); } void psbt_elements_input_set_asset(struct wally_psbt *psbt, size_t in, @@ -308,6 +325,7 @@ void psbt_elements_input_set_asset(struct wally_psbt *psbt, size_t in, asset->asset + 1, ELEMENTS_ASSET_LEN - 1) != WALLY_OK) abort(); + tal_gather_wally(psbt); } void psbt_elements_normalize_fees(struct wally_psbt *psbt) @@ -352,6 +370,7 @@ void psbt_elements_normalize_fees(struct wally_psbt *psbt) /* We need to add a fee output */ if (fee_output_idx == psbt->num_outputs) { psbt_append_output(psbt, NULL, total_in); + tal_gather_wally(psbt); } else { u64 sats = total_in.satoshis; /* Raw: wally API */ struct wally_tx_output *out = &psbt->tx->outputs[fee_output_idx]; @@ -475,6 +494,7 @@ void psbt_input_add_unknown(const tal_t *ctx, (unsigned char *) memcheck(value, value_len), value_len) != WALLY_OK) abort(); + tal_gather_wally(ctx); } void *psbt_get_unknown(const struct wally_map *map, @@ -518,6 +538,7 @@ void psbt_output_add_unknown(const tal_t *ctx, (unsigned char *) memcheck(value, value_len), value_len) != WALLY_OK) abort(); + tal_gather_wally(ctx); } /* Use the destructor to free the wally_tx */ @@ -538,7 +559,8 @@ struct wally_tx *psbt_finalize(const tal_t *ctx, if (!finalize_in_place) { if (wally_psbt_clone_alloc(psbt, 0, &tmppsbt) != WALLY_OK) return NULL; - tal_add_destructor(tmppsbt, psbt_destroy); + /* Explicitly freed below */ + tal_steal(NULL, tmppsbt); } else tmppsbt = cast_const(struct wally_psbt *, psbt); @@ -578,6 +600,7 @@ struct wally_tx *psbt_finalize(const tal_t *ctx, input->witness_script, input->witness_script_len); input->final_witness = stack; + tal_gather_wally(tmppsbt); } if (wally_psbt_finalize(tmppsbt) != WALLY_OK) { @@ -586,9 +609,13 @@ struct wally_tx *psbt_finalize(const tal_t *ctx, return NULL; } + /* wally_psbt_finalize() may or may not have done allocations! */ + if (tal_first(wally_tal_ctx)) + tal_gather_wally(tmppsbt); + if (psbt_is_finalized(tmppsbt) && wally_psbt_extract(tmppsbt, &wtx) == WALLY_OK) { - tal_steal(ctx, wtx); + tal_gather_wally(tal_steal(ctx, wtx)); tal_add_destructor(wtx, wally_tx_destroy); if (!finalize_in_place) wally_psbt_free(tmppsbt); @@ -611,7 +638,7 @@ struct wally_psbt *psbt_from_b64(const tal_t *ctx, return NULL; /* We promised it would be owned by ctx: libwally uses a dummy owner */ - tal_steal(ctx, psbt); + tal_gather_wally(tal_steal(ctx, psbt)); tal_add_destructor(psbt, psbt_destroy); return psbt; } @@ -661,7 +688,7 @@ struct wally_psbt *psbt_from_bytes(const tal_t *ctx, const u8 *bytes, return NULL; /* We promised it would be owned by ctx: libwally uses a dummy owner */ - tal_steal(ctx, psbt); + tal_gather_wally(tal_steal(ctx, psbt)); tal_add_destructor(psbt, psbt_destroy); return psbt; } @@ -732,10 +759,11 @@ void psbt_txid(const tal_t *ctx, psbt->inputs[i].redeem_script_len); wally_tx_set_input_script(tx, i, script, tal_bytelen(script)); } + tal_gather_wally(tal_steal(ctx, tx)); wally_txid(tx, txid); if (wtx) - *wtx = tal_steal(ctx, tx); + *wtx = tx; else wally_tx_free(tx); } diff --git a/bitcoin/tx.c b/bitcoin/tx.c index e8b513def049..bad96156ad69 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -54,7 +54,9 @@ struct wally_tx_output *wally_tx_output(const tal_t *ctx, if (ret != WALLY_OK) return NULL; } - return tal_steal(ctx, output); + + tal_gather_wally(tal_steal(ctx, output)); + return output; } int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script, @@ -74,6 +76,7 @@ int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script, assert(output); ret = wally_tx_add_output(tx->wtx, output); assert(ret == WALLY_OK); + tal_gather_wally(tx->wtx); psbt_out = psbt_add_output(tx->psbt, output, i); if (wscript) { @@ -81,6 +84,7 @@ int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script, wscript, tal_bytelen(wscript)); assert(ret == WALLY_OK); + tal_gather_wally(tx->psbt); } wally_tx_output_free(output); @@ -213,6 +217,8 @@ int bitcoin_tx_add_input(struct bitcoin_tx *tx, const struct bitcoin_txid *txid, /* scriptsig isn't actually stored in psbt input, so add that now */ wally_tx_set_input_script(tx->wtx, input_num, scriptSig, tal_bytelen(scriptSig)); + if (scriptSig) + tal_gather_wally(tx->wtx); if (is_elements(chainparams)) { struct amount_asset asset; @@ -331,11 +337,14 @@ void bitcoin_tx_input_set_witness(struct bitcoin_tx *tx, int innum, tal_bytelen(witness[i])); } wally_tx_set_input_witness(tx->wtx, innum, stack); + if (stack) + tal_gather_wally(tx->wtx); /* Also add to the psbt */ - if (stack) + if (stack) { wally_psbt_input_set_final_witness(&tx->psbt->inputs[innum], stack); - else { + tal_gather_wally(tx->psbt); + } else { /* FIXME: libwally-psbt doesn't allow 'unsetting' of witness via * the set method at the moment, so we do it manually*/ struct wally_psbt_input *in = &tx->psbt->inputs[innum]; @@ -354,11 +363,13 @@ void bitcoin_tx_input_set_script(struct bitcoin_tx *tx, int innum, u8 *script) { struct wally_psbt_input *in; wally_tx_set_input_script(tx->wtx, innum, script, tal_bytelen(script)); + tal_gather_wally(tx->wtx); /* Also add to the psbt */ assert(innum < tx->psbt->num_inputs); in = &tx->psbt->inputs[innum]; wally_psbt_input_set_final_scriptsig(in, script, tal_bytelen(script)); + tal_gather_wally(tx->psbt); } const u8 *bitcoin_tx_input_get_witness(const tal_t *ctx, @@ -481,6 +492,7 @@ struct bitcoin_tx *bitcoin_tx(const tal_t *ctx, wally_tx_init_alloc(WALLY_TX_VERSION_2, nlocktime, input_count, output_count, &tx->wtx); + tal_gather_wally(tal_steal(tx, tx->wtx)); tal_add_destructor(tx, bitcoin_tx_destroy); tx->chainparams = chainparams; @@ -503,8 +515,11 @@ struct bitcoin_tx *bitcoin_tx_with_psbt(const tal_t *ctx, struct wally_psbt *psb psbt->tx->locktime); wally_tx_free(tx->wtx); tx->wtx = psbt_finalize(tx, psbt, false); - if (!tx->wtx && wally_tx_clone_alloc(psbt->tx, 0, &tx->wtx) != WALLY_OK) - return NULL; + if (!tx->wtx) { + if (wally_tx_clone_alloc(psbt->tx, 0, &tx->wtx) != WALLY_OK) + return NULL; + tal_gather_wally(tal_steal(tx, tx->wtx)); + } tal_free(tx->psbt); tx->psbt = tal_steal(tx, psbt); @@ -527,6 +542,7 @@ struct bitcoin_tx *pull_bitcoin_tx(const tal_t *ctx, const u8 **cursor, return tal_free(tx); } + tal_gather_wally(tx); tal_add_destructor(tx, bitcoin_tx_destroy); wally_tx_get_length(tx->wtx, flags, &wsize); diff --git a/bitcoin/tx_parts.c b/bitcoin/tx_parts.c index a825d86018d3..57da6ffa8efc 100644 --- a/bitcoin/tx_parts.c +++ b/bitcoin/tx_parts.c @@ -39,8 +39,9 @@ static struct wally_tx_input *clone_input(const tal_t *ctx, } assert(ret == WALLY_OK); + tal_gather_wally(tal_steal(ctx, in)); tal_add_destructor(in, destroy_wally_tx_input); - return tal_steal(ctx, in); + return in; } static void destroy_wally_tx_output(struct wally_tx_output *out) @@ -70,8 +71,9 @@ static struct wally_tx_output *clone_output(const tal_t *ctx, } assert(ret == WALLY_OK); + tal_gather_wally(tal_steal(ctx, out)); tal_add_destructor(out, destroy_wally_tx_output); - return tal_steal(ctx, out); + return out; } struct tx_parts *tx_parts_from_wally_tx(const tal_t *ctx, @@ -99,6 +101,11 @@ struct tx_parts *tx_parts_from_wally_tx(const tal_t *ctx, return txp; } +static void destroy_wally_tx_witness_stack(struct wally_tx_witness_stack *ws) +{ + wally_tx_witness_stack_free(ws); +} + /* FIXME: If libwally exposed their linearization code, we could use it */ static struct wally_tx_witness_stack * fromwire_wally_tx_witness_stack(const tal_t *ctx, @@ -131,6 +138,8 @@ fromwire_wally_tx_witness_stack(const tal_t *ctx, } } + tal_gather_wally(tal_steal(ctx, ws)); + tal_add_destructor(ws, destroy_wally_tx_witness_stack); return ws; } @@ -225,8 +234,9 @@ static struct wally_tx_input *fromwire_wally_tx_input(const tal_t *ctx, return NULL; } + tal_gather_wally(tal_steal(ctx, in)); tal_add_destructor(in, destroy_wally_tx_input); - return tal_steal(ctx, in); + return in; } static struct wally_tx_output *fromwire_wally_tx_output(const tal_t *ctx, @@ -278,8 +288,9 @@ static struct wally_tx_output *fromwire_wally_tx_output(const tal_t *ctx, return NULL; } + tal_gather_wally(tal_steal(ctx, out)); tal_add_destructor(out, destroy_wally_tx_output); - return tal_steal(ctx, out); + return out; } static void towire_wally_tx_input(u8 **pptr, const struct wally_tx_input *in) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index b5e4010dbaca..70fd76c53a92 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -1594,6 +1594,7 @@ static void sign_our_inputs(struct utxo **utxos, struct wally_psbt *psbt) } } + tal_gather_wally(psbt); } /*~ lightningd asks us to sign a withdrawal; same as above but in theory From 8a9d7d5d29bf7bc54f3a939e164250932232e0ce Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 10/15] common: enforce that we have collected all wally allocations. Signed-off-by: Rusty Russell --- common/daemon.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/common/daemon.c b/common/daemon.c index 4b76f9b65041..c19bb86bf0c5 100644 --- a/common/daemon.c +++ b/common/daemon.c @@ -81,11 +81,20 @@ void send_backtrace(const char *why) int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout) { const char *t; + char *wally_leak; t = taken_any(); if (t) errx(1, "Outstanding taken pointers: %s", t); + wally_leak = tal_first(wally_tal_ctx); + if (wally_leak) { + /* Trigger valgrind to tell us about this! */ + tal_free(wally_leak); + *wally_leak = 0; + errx(1, "Outstanding wally allocations"); + } + clean_tmpctx(); return poll(fds, nfds, timeout); From b8f6a3cbfaaf75c6c51fe42b6cc7d5d48d02f0a1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 11/15] memleak: make "_notleak" names less powerful. They previously prevented any child from being detected as leaks, now they just mark the tal allocation itself as not being a leak. Signed-off-by: Rusty Russell --- common/configdir.c | 3 +++ common/memleak.c | 7 +++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/common/configdir.c b/common/configdir.c index 15027e03b96c..e47777625de5 100644 --- a/common/configdir.c +++ b/common/configdir.c @@ -122,6 +122,9 @@ static void parse_include(const char *filename, bool must_exist, bool early, /* Only valid forms are "foo" and "foo=bar" */ all_args[i] = tal_fmt(all_args, "--%s", lines[i]); } + /* This isn't a leak either */ + if (all_args[i]) + tal_set_name(all_args[i], TAL_LABEL(config_notleak, "")); } /* diff --git a/common/memleak.c b/common/memleak.c index 84c3bb6f71bb..13e33699643c 100644 --- a/common/memleak.c +++ b/common/memleak.c @@ -77,10 +77,6 @@ static void children_into_htable(const void *exclude1, const void *exclude2, if (strends(name, "struct io_plan *[]") && !tal_parent(i)) continue; - /* Other notleak internals. */ - if (strends(name, "_notleak")) - continue; - /* Don't add tmpctx. */ if (streq(name, "tmpctx")) continue; @@ -244,6 +240,9 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p) else pointer_referenced(memtable, p); memleak_scan_region(memtable, p, tal_bytelen(p)); + } else if (name && strends(name, "_notleak")) { + pointer_referenced(memtable, i); + call_memleak_helpers(memtable, i); } else { call_memleak_helpers(memtable, i); } From 2fa1986a4b945e26766ca02ecadeccf905cff964 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 12/15] common: don't suppress leak detection for libwally allocations. Signed-off-by: Rusty Russell --- common/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/setup.c b/common/setup.c index f0d5c0d5fba8..ad971b3683b1 100644 --- a/common/setup.c +++ b/common/setup.c @@ -7,7 +7,7 @@ static void *wally_tal(size_t size) { - return tal_arr_label(wally_tal_ctx, u8, size, "wally_notleak"); + return tal_arr_label(wally_tal_ctx, u8, size, "wally_tal"); } static void wally_free(void *ptr) From 77099906adbe6850b32336e47fe3fec97ffce640 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 13/15] pytest: don't test for memleaks under valgrind. The next patch perturbed things enough that we suddenly started getting (with --track-origins=yes): Valgrind error file: valgrind-errors.120470 ==120470== Use of uninitialised value of size 8 ==120470== at 0x14EBD5: htable_val (htable.c:150) ==120470== by 0x14EC3C: htable_firstval_ (htable.c:165) ==120470== by 0x14F583: htable_del_ (htable.c:349) ==120470== by 0x11825D: pointer_referenced (memleak.c:65) ==120470== by 0x118485: scan_for_pointers (memleak.c:121) ==120470== by 0x118500: memleak_remove_region (memleak.c:130) ==120470== by 0x118A30: call_memleak_helpers (memleak.c:257) ==120470== by 0x118A8B: call_memleak_helpers (memleak.c:262) ==120470== by 0x118A8B: call_memleak_helpers (memleak.c:262) ==120470== by 0x118B25: memleak_find_allocations (memleak.c:278) ==120470== by 0x10EB12: closing_dev_memleak (closingd.c:584) ==120470== by 0x10F3E2: main (closingd.c:783) ==120470== Uninitialised value was created by a heap allocation ==120470== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==120470== by 0x1604E8: allocate (tal.c:250) ==120470== by 0x160AA9: tal_alloc_ (tal.c:428) ==120470== by 0x119BE0: new_per_peer_state (per_peer_state.c:24) ==120470== by 0x11A101: fromwire_per_peer_state (per_peer_state.c:95) ==120470== by 0x10FB7C: fromwire_closingd_init (closingd_wiregen.c:103) ==120470== by 0x10ED15: main (closingd.c:626) ==120470== This is because there is uninitialized padding at the end of struct peer_state. Signed-off-by: Rusty Russell --- common/memleak.c | 10 ++++++---- contrib/pyln-testing/pyln/testing/utils.py | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/common/memleak.c b/common/memleak.c index 13e33699643c..03a7d96c3cf5 100644 --- a/common/memleak.c +++ b/common/memleak.c @@ -256,11 +256,13 @@ struct htable *memleak_enter_allocations(const tal_t *ctx, struct htable *memtable = tal(ctx, struct htable); htable_init(memtable, hash_ptr, NULL); - /* First, add all pointers off NULL to table. */ - children_into_htable(exclude1, exclude2, memtable, NULL); + if (memleak_track) { + /* First, add all pointers off NULL to table. */ + children_into_htable(exclude1, exclude2, memtable, NULL); - /* Iterate and call helpers to eliminate hard-to-get references. */ - call_memleak_helpers(memtable, NULL); + /* Iterate and call helpers to eliminate hard-to-get references. */ + call_memleak_helpers(memtable, NULL); + } tal_add_destructor(memtable, htable_clear); return memtable; diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 48b0b8a6dd87..be34ba7f7444 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -584,11 +584,13 @@ def __init__(self, node_id, lightning_dir, bitcoind, executor, valgrind, may_fai # Don't run --version on every subdaemon if we're valgrinding and slow. if SLOW_MACHINE and VALGRIND: self.daemon.opts["dev-no-version-checks"] = None - self.daemon.env["LIGHTNINGD_DEV_MEMLEAK"] = "1" if os.getenv("DEBUG_SUBD"): self.daemon.opts["dev-debugger"] = os.getenv("DEBUG_SUBD") if valgrind: self.daemon.env["LIGHTNINGD_DEV_NO_BACKTRACE"] = "1" + else: + # Under valgrind, scanning can access uninitialized mem. + self.daemon.env["LIGHTNINGD_DEV_MEMLEAK"] = "1" if not may_reconnect: self.daemon.opts["dev-no-reconnect"] = None From 20a2476d7c1c3cf6bb7b43b14adb1751bce90e3d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 14/15] common/memleak: simplify and document API. 1. Rename memleak_enter_allocations to memleak_find_allocations. 2. Unify scanning for pointers into memleak_remove_region / memleak_remove_pointer. 3. Document the functions. Signed-off-by: Rusty Russell --- channeld/channeld.c | 4 +- closingd/closingd.c | 14 ++-- common/memleak.c | 49 ++++++++---- common/memleak.h | 109 +++++++++++++++++++------- connectd/connectd.c | 4 +- gossipd/gossipd.c | 4 +- hsmd/hsmd.c | 15 ++-- lightningd/memdump.c | 6 +- onchaind/onchaind.c | 27 ++----- onchaind/test/run-grind_feerate-bug.c | 21 +++-- onchaind/test/run-grind_feerate.c | 21 +++-- openingd/dualopend.c | 4 +- openingd/openingd.c | 4 +- 13 files changed, 166 insertions(+), 116 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index a48df2b0b7d3..25cfdac98bb7 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -3088,10 +3088,10 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg) struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete peer and things it has pointers to. */ - memleak_remove_referenced(memtable, peer); + memleak_remove_region(memtable, peer, tal_bytelen(peer)); found_leak = dump_memleak(memtable); wire_sync_write(MASTER_FD, diff --git a/closingd/closingd.c b/closingd/closingd.c index f2f19bca2c7c..517481cb2080 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -581,14 +581,12 @@ static void closing_dev_memleak(const tal_t *ctx, { struct htable *memtable; - memtable = memleak_enter_allocations(tmpctx, - scriptpubkey[LOCAL], - scriptpubkey[REMOTE]); + memtable = memleak_find_allocations(tmpctx, NULL, NULL); - /* Now delete known pointers (these aren't really roots, just - * pointers we know are referenced).*/ - memleak_remove_referenced(memtable, ctx); - memleak_remove_referenced(memtable, funding_wscript); + memleak_remove_pointer(memtable, ctx); + memleak_remove_pointer(memtable, scriptpubkey[LOCAL]); + memleak_remove_pointer(memtable, scriptpubkey[REMOTE]); + memleak_remove_pointer(memtable, funding_wscript); dump_memleak(memtable); } @@ -653,7 +651,7 @@ int main(int argc, char *argv[]) master_badmsg(WIRE_CLOSINGD_INIT, msg); /* stdin == requests, 3 == peer, 4 = gossip, 5 = gossip_store, 6 = hsmd */ - per_peer_state_set_fds(pps, 3, 4, 5); + per_peer_state_set_fds(notleak(pps), 3, 4, 5); snprintf(fee_negotiation_step_str, sizeof(fee_negotiation_step_str), "%" PRIu64 "%s", fee_negotiation_step, diff --git a/common/memleak.c b/common/memleak.c index 03a7d96c3cf5..3a8852bf9663 100644 --- a/common/memleak.c +++ b/common/memleak.c @@ -1,3 +1,24 @@ +/* Hello friends! + * + * You found me! This is the inner, deep magic. Right here. + * + * To help development, we have a complete set of routines to scan for + * tal-memory leaks (valgrind will detect non-tal memory leaks at exit, + * but tal hierarchies tends to get freed at exit, so we need something + * more sophisticated). + * + * Memleak detection is only active if DEVELOPER is set. It does several + * things: + * 1. attaches a backtrace list to every allocation, so we can tell + * where it came from. + * 2. when memleak_find_allocations() is called, walks the entire tal + * tree and saves a pointer to all the objects it finds, with + * a few internal exceptions (including everything under 'tmpctx'). + * It then calls registered helpers, which can remove opaque things + * and handles notleak() objects. + * 3. provides a routine to access any remaining pointers in the + * table: these are the leaks. + */ #include #include #include @@ -102,8 +123,8 @@ static void scan_for_pointers(struct htable *memtable, } } -void memleak_scan_region(struct htable *memtable, - const void *ptr, size_t bytelen) +void memleak_remove_region(struct htable *memtable, + const void *ptr, size_t bytelen) { pointer_referenced(memtable, ptr); scan_for_pointers(memtable, ptr, bytelen); @@ -118,15 +139,6 @@ static void remove_with_children(struct htable *memtable, const tal_t *p) remove_with_children(memtable, i); } -void memleak_remove_referenced(struct htable *memtable, const void *root) -{ - /* Now delete the ones which are referenced. */ - memleak_scan_region(memtable, root, tal_bytelen(root)); - - /* Remove memtable itself */ - pointer_referenced(memtable, memtable); -} - /* memleak can't see inside hash tables, so do them manually */ void memleak_remove_htable(struct htable *memtable, const struct htable *ht) { @@ -134,7 +146,7 @@ void memleak_remove_htable(struct htable *memtable, const struct htable *ht) const void *p; for (p = htable_first(ht, &i); p; p = htable_next(ht, &i)) - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } /* FIXME: If uintmap used tal, this wouldn't be necessary! */ @@ -144,7 +156,7 @@ void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m) intmap_index_t i; for (p = intmap_first_(m, &i); p; p = intmap_after_(m, &i)) - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } static bool ptr_match(const void *candidate, void *ptr) @@ -157,6 +169,9 @@ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace) struct htable_iter it; const tal_t *i, *p; + /* Remove memtable itself */ + pointer_referenced(memtable, memtable); + i = htable_first(memtable, &it); if (!i) return NULL; @@ -239,7 +254,7 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p) remove_with_children(memtable, p); else pointer_referenced(memtable, p); - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } else if (name && strends(name, "_notleak")) { pointer_referenced(memtable, i); call_memleak_helpers(memtable, i); @@ -249,9 +264,9 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p) } } -struct htable *memleak_enter_allocations(const tal_t *ctx, - const void *exclude1, - const void *exclude2) +struct htable *memleak_find_allocations(const tal_t *ctx, + const void *exclude1, + const void *exclude2) { struct htable *memtable = tal(ctx, struct htable); htable_init(memtable, hash_ptr, NULL); diff --git a/common/memleak.h b/common/memleak.h index 8852b99e34ce..1afa9725ce28 100644 --- a/common/memleak.h +++ b/common/memleak.h @@ -8,26 +8,48 @@ struct htable; +/** + * memleak_init: Initialize memleak detection; you call this at the start! + * + * notleak() won't have an effect if called before this (but naming + * tal objects with suffix _notleak works). + */ +void memleak_init(void); + +/** + * notleak: remove a false-positive tal object. + * @p: the tal allocation. + * + * This marks a tal pointer (and anything it refers to) as not being + * leaked. Think hard before using this! + */ +#define notleak(p) ((memleak_typeof(p))notleak_((p), false)) + +/* Mark a pointer and all its tal children as not being leaked. + * You don't want this; it's for simplifying handling of the incoming + * command which asks lightningd to do the dev check. */ +#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true)) + #if HAVE_TYPEOF #define memleak_typeof(var) typeof(var) #else #define memleak_typeof(var) void * #endif /* !HAVE_TYPEOF */ -/* Mark a pointer as not being leaked. */ -#define notleak(p) ((memleak_typeof(p))notleak_((p), false)) - -/* Mark a pointer and all its tal children as not being leaked. */ -#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true)) - void *notleak_(const void *ptr, bool plus_children); -/* Mark a helper to be called to scan this structure for mem references */ -/* For update-mock: memleak_add_helper_ mock empty */ -void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, - const tal_t *)); - #if DEVELOPER +/** + * memleak_add_helper: help memleak look inside this tal object + * @p: the tal object + * @cb: the callback. + * + * memleak looks for tal pointers inside a tal object memory, but some + * structures which use bit-stealing on pointers or use non-tal allocations + * will need this. + * + * The callback usually calls memleak_remove_*. + */ #define memleak_add_helper(p, cb) \ memleak_add_helper_((p), \ typesafe_cb_preargs(void, const tal_t *, \ @@ -38,32 +60,63 @@ void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, #define memleak_add_helper(p, cb) #endif -/* Initialize memleak detection */ -void memleak_init(void); - -/* Allocate a htable with all the memory we've allocated. */ -struct htable *memleak_enter_allocations(const tal_t *ctx, - const void *exclude1, - const void *exclude2); - -/* Remove any pointers to memory under root */ -void memleak_remove_referenced(struct htable *memtable, const void *root); +/* For update-mock: memleak_add_helper_ mock empty */ +void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, + const tal_t *)); -/* Remove any pointers inside this htable (which is opaque to memleak). */ +/** + * memleak_find_allocations: allocate a htable with all tal objects; + * @ctx: the context to allocate the htable from + * @exclude1: one tal pointer to exclude from adding (if non-NULL) + * @exclude2: second tal pointer to exclude from adding (if non-NULL) + * + * Note that exclude1 and exclude2's tal children are also not added. + */ +struct htable *memleak_find_allocations(const tal_t *ctx, + const void *exclude1, + const void *exclude2); + +/** + * memleak_remove_region - remove this region and anything it references + * @memtable: the memtable create by memleak_find_allocations. + * @p: the pointer to remove. + * @bytelen: the bytes within it to scan for more pointers. + * + * This removes @p from the memtable, then looks for any tal pointers + * inside between @p and @p + @bytelen and calls + * memleak_remove_region() on those if not already removed. + */ +void memleak_remove_region(struct htable *memtable, + const void *p, size_t bytelen); + +/** + * memleak_remove_pointer - remove this pointer + * @memtable: the memtable create by memleak_find_allocations. + * @p: the pointer to remove. + * + * This removes @p from the memtable. + */ +#define memleak_remove_pointer(memtable, p) \ + memleak_remove_region((memtable), (p), 0) + +/* Helper to remove objects inside this htable (which is opaque to memleak). */ void memleak_remove_htable(struct htable *memtable, const struct htable *ht); -/* Remove any pointers inside this uintmap (which is opaque to memleak). */ +/* Helper to remove objects inside this uintmap (which is opaque to memleak). */ #define memleak_remove_uintmap(memtable, umap) \ memleak_remove_intmap_(memtable, uintmap_unwrap_(umap)) struct intmap; void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m); -/* Mark this pointer as being referenced, and search within for more. */ -void memleak_scan_region(struct htable *memtable, - const void *p, size_t bytelen); - -/* Get (and remove) a leak from memtable, or NULL */ +/** + * memleak_get: get (and remove) a leak from memtable, or NULL + * @memtable: the memtable after all known allocations removed. + * @backtrace: the backtrace to set if there is one. + * + * If this returns NULL, it means the @memtable was empty. Otherwise + * it return a pointer to a leak (and removes it from @memtable) + */ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace); extern struct backtrace_state *backtrace_state; diff --git a/connectd/connectd.c b/connectd/connectd.c index 15798373324e..986ea2511050 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -1571,10 +1571,10 @@ static struct io_plan *dev_connect_memleak(struct io_conn *conn, struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete daemon and those which it has pointers to. */ - memleak_remove_referenced(memtable, daemon); + memleak_remove_region(memtable, daemon, sizeof(daemon)); found_leak = dump_memleak(memtable); daemon_conn_send(daemon->master, diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index ceeaaba59aea..1334c290a9c5 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1346,10 +1346,10 @@ static struct io_plan *dev_gossip_memleak(struct io_conn *conn, struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete daemon and those which it has pointers to. */ - memleak_remove_referenced(memtable, daemon); + memleak_remove_region(memtable, daemon, sizeof(*daemon)); found_leak = dump_memleak(memtable); daemon_conn_send(daemon->master, diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 70fd76c53a92..2cfde59424d4 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -1799,17 +1799,18 @@ static struct io_plan *handle_memleak(struct io_conn *conn, bool found_leak; u8 *reply; - memtable = memleak_enter_allocations(tmpctx, msg_in, msg_in); + memtable = memleak_find_allocations(tmpctx, msg_in, msg_in); /* Now delete clients and anything they point to. */ - memleak_remove_referenced(memtable, c); - memleak_scan_region(memtable, - dbid_zero_clients, sizeof(dbid_zero_clients)); + memleak_remove_region(memtable, c, tal_bytelen(c)); + memleak_remove_region(memtable, + dbid_zero_clients, sizeof(dbid_zero_clients)); memleak_remove_uintmap(memtable, &clients); - memleak_scan_region(memtable, status_conn, tal_bytelen(status_conn)); + memleak_remove_region(memtable, + status_conn, tal_bytelen(status_conn)); - memleak_scan_region(memtable, dev_force_privkey, 0); - memleak_scan_region(memtable, dev_force_bip32_seed, 0); + memleak_remove_pointer(memtable, dev_force_privkey); + memleak_remove_pointer(memtable, dev_force_bip32_seed); found_leak = dump_memleak(memtable); reply = towire_hsmd_dev_memleak_reply(NULL, found_leak); diff --git a/lightningd/memdump.c b/lightningd/memdump.c index ba5cafbf83f4..2ddda59cb1ba 100644 --- a/lightningd/memdump.c +++ b/lightningd/memdump.c @@ -132,7 +132,7 @@ static bool handle_strmap(const char *member, void *p, void *memtable_) { struct htable *memtable = memtable_; - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); /* Keep going */ return true; @@ -154,7 +154,7 @@ static void scan_mem(struct command *cmd, const uintptr_t *backtrace; /* Enter everything, except this cmd and its jcon */ - memtable = memleak_enter_allocations(cmd, cmd, cmd->jcon); + memtable = memleak_find_allocations(cmd, cmd, cmd->jcon); /* First delete known false positives. */ memleak_remove_htable(memtable, &ld->topology->txwatches.raw); @@ -164,7 +164,7 @@ static void scan_mem(struct command *cmd, memleak_remove_htable(memtable, &ld->htlc_sets.raw); /* Now delete ld and those which it has pointers to. */ - memleak_remove_referenced(memtable, ld); + memleak_remove_region(memtable, ld, sizeof(*ld)); json_array_start(response, "leaks"); while ((i = memleak_get(memtable, &backtrace)) != NULL) { diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index f0f055c14210..e61203554b58 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -2087,24 +2087,13 @@ static void handle_preimage(struct tracked_output **outs, #if DEVELOPER static void memleak_remove_globals(struct htable *memtable, const tal_t *topctx) { - /* memleak_scan_region is overkill if these are simple pointers to - * objects which don't contain pointers, but it works. */ if (keyset) - memleak_scan_region(memtable, keyset, sizeof(*keyset)); - - if (remote_per_commitment_point) - memleak_scan_region(memtable, remote_per_commitment_point, - sizeof(*remote_per_commitment_point)); - - if (remote_per_commitment_secret) - memleak_scan_region(memtable, remote_per_commitment_secret, - sizeof(*remote_per_commitment_secret)); - - /* top-level context args */ - memleak_scan_region(memtable, topctx, 0); - - memleak_scan_region(memtable, missing_htlc_msgs, - tal_bytelen(missing_htlc_msgs)); + memleak_remove_region(memtable, keyset, sizeof(*keyset)); + memleak_remove_pointer(memtable, remote_per_commitment_point); + memleak_remove_pointer(memtable, remote_per_commitment_secret); + memleak_remove_pointer(memtable, topctx); + memleak_remove_region(memtable, + missing_htlc_msgs, tal_bytelen(missing_htlc_msgs)); } static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg) @@ -2115,10 +2104,10 @@ static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg) if (!fromwire_onchaind_dev_memleak(msg)) return false; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Top-level context is parent of outs */ memleak_remove_globals(memtable, tal_parent(outs)); - memleak_remove_referenced(memtable, outs); + memleak_remove_region(memtable, outs, tal_bytelen(outs)); found_leak = dump_memleak(memtable); wire_sync_write(REQ_FD, diff --git a/onchaind/test/run-grind_feerate-bug.c b/onchaind/test/run-grind_feerate-bug.c index 9713db507b9b..9fd8f3b8cdd4 100644 --- a/onchaind/test/run-grind_feerate-bug.c +++ b/onchaind/test/run-grind_feerate-bug.c @@ -112,18 +112,15 @@ struct bitcoin_tx *htlc_success_tx(const tal_t *ctx UNNEEDED, /* Generated stub for master_badmsg */ void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg) { fprintf(stderr, "master_badmsg called!\n"); abort(); } -/* Generated stub for memleak_enter_allocations */ -struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED, - const void *exclude1 UNNEEDED, - const void *exclude2 UNNEEDED) -{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); } -/* Generated stub for memleak_remove_referenced */ -void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED) -{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); } -/* Generated stub for memleak_scan_region */ -void memleak_scan_region(struct htable *memtable UNNEEDED, - const void *p UNNEEDED, size_t bytelen UNNEEDED) -{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); } +/* Generated stub for memleak_find_allocations */ +struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED, + const void *exclude1 UNNEEDED, + const void *exclude2 UNNEEDED) +{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); } +/* Generated stub for memleak_remove_region */ +void memleak_remove_region(struct htable *memtable UNNEEDED, + const void *p UNNEEDED, size_t bytelen UNNEEDED) +{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); } /* Generated stub for new_coin_chain_fees */ struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED, const char *account_name UNNEEDED, diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 1f9b3a13e4db..c1168f4db784 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -126,18 +126,15 @@ struct bitcoin_tx *htlc_timeout_tx(const tal_t *ctx UNNEEDED, /* Generated stub for master_badmsg */ void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg) { fprintf(stderr, "master_badmsg called!\n"); abort(); } -/* Generated stub for memleak_enter_allocations */ -struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED, - const void *exclude1 UNNEEDED, - const void *exclude2 UNNEEDED) -{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); } -/* Generated stub for memleak_remove_referenced */ -void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED) -{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); } -/* Generated stub for memleak_scan_region */ -void memleak_scan_region(struct htable *memtable UNNEEDED, - const void *p UNNEEDED, size_t bytelen UNNEEDED) -{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); } +/* Generated stub for memleak_find_allocations */ +struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED, + const void *exclude1 UNNEEDED, + const void *exclude2 UNNEEDED) +{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); } +/* Generated stub for memleak_remove_region */ +void memleak_remove_region(struct htable *memtable UNNEEDED, + const void *p UNNEEDED, size_t bytelen UNNEEDED) +{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); } /* Generated stub for new_coin_chain_fees */ struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED, const char *account_name UNNEEDED, diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 0810a73c6682..d8f990ac0722 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -1223,10 +1223,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) /* Populate a hash table with all our allocations (except msg, which * is in use right now). */ - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete state and things it has pointers to. */ - memleak_remove_referenced(memtable, state); + memleak_remove_region(memtable, state, tal_bytelen(state)); /* If there's anything left, dump it to logs, and return true. */ found_leak = dump_memleak(memtable); diff --git a/openingd/openingd.c b/openingd/openingd.c index db83ab0ce6d8..75e1c6956f9a 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -1262,10 +1262,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) /* Populate a hash table with all our allocations (except msg, which * is in use right now). */ - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete state and things it has pointers to. */ - memleak_remove_referenced(memtable, state); + memleak_remove_region(memtable, state, sizeof(*state)); /* If there's anything left, dump it to logs, and return true. */ found_leak = dump_memleak(memtable); From d17a3c5ac92f8a4adc3312cffb53b4709bb017c4 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH 15/15] topo: Do not keep txids in memory indefinitely I mistakenly assumed the block would be freed after processing completed. That is not true since chaintopology keeps headers and stubs around for reorgs. So we need to remove the precomputed txids along with the full_txs. --- lightningd/chaintopology.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 4c16f72c542a..7c4bdc11c838 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -108,6 +108,7 @@ static void filter_block_txs(struct chain_topology *topo, struct block *b) txwatch_inform(topo, &txid, tx); } b->full_txs = tal_free(b->full_txs); + b->txids = tal_free(b->txids); } size_t get_tx_depth(const struct chain_topology *topo,