From 65be18d355c544c22f00fef4e8c4d46d2efceacc Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 20 Mar 2022 12:59:27 +1030 Subject: [PATCH] memleak: handle libwally allocations better. Things allocated by libwally all get the tal_name "wally_tal", which cost me a few hours trying to find a leak. In the case where we're making one of the allocations the parent of the others (e.g. a wally_psbt), we can do better: supply a name for the tal_wally_end(). So I add a new tal_wally_end_onto() which does the standard tal_steal() trick, and also changes the (typechecked!) name. Signed-off-by: Rusty Russell --- bitcoin/base58.c | 2 +- bitcoin/psbt.c | 16 ++++++++-------- bitcoin/tx.c | 8 ++++---- bitcoin/tx_parts.c | 6 +++--- common/utils.c | 25 +++++++++++++++++-------- common/utils.h | 15 +++++++++++++-- lightningd/dual_open_control.c | 6 ++++-- plugins/spender/multifundchannel.c | 2 +- plugins/spender/openchannel.c | 8 ++++---- 9 files changed, 55 insertions(+), 33 deletions(-) diff --git a/bitcoin/base58.c b/bitcoin/base58.c index 0af71d8f3..d1368289d 100644 --- a/bitcoin/base58.c +++ b/bitcoin/base58.c @@ -26,7 +26,7 @@ static char *to_base58(const tal_t *ctx, u8 version, total_length, BASE58_FLAG_CHECKSUM, &out) != WALLY_OK) out = NULL; - tal_wally_end(tal_steal(ctx, out)); + tal_wally_end_onto(ctx, out, char); return out; } diff --git a/bitcoin/psbt.c b/bitcoin/psbt.c index f3fda99ed..990621104 100644 --- a/bitcoin/psbt.c +++ b/bitcoin/psbt.c @@ -29,7 +29,7 @@ static struct wally_psbt *init_psbt(const tal_t *ctx, size_t num_inputs, size_t wally_err = wally_psbt_init_alloc(0, num_inputs, num_outputs, 0, &psbt); assert(wally_err == WALLY_OK); tal_add_destructor(psbt, psbt_destroy); - tal_wally_end(tal_steal(ctx, psbt)); + tal_wally_end_onto(ctx, psbt, struct wally_psbt); return psbt; } @@ -63,7 +63,7 @@ struct wally_psbt *clone_psbt(const tal_t *ctx, struct wally_psbt *psbt) tal_wally_start(); if (wally_psbt_clone_alloc(psbt, 0, &clone) != WALLY_OK) abort(); - tal_wally_end(tal_steal(ctx, clone)); + tal_wally_end_onto(ctx, clone, struct wally_psbt); return clone; } @@ -634,7 +634,7 @@ bool psbt_finalize(struct wally_psbt *psbt) wally_tx_witness_stack_add(stack, input->witness_script, input->witness_script_len); - input->final_witness = stack; + wally_psbt_input_set_final_witness(input, stack); } ok = (wally_psbt_finalize(psbt) == WALLY_OK); @@ -656,7 +656,7 @@ struct wally_tx *psbt_final_tx(const tal_t *ctx, const struct wally_psbt *psbt) else wtx = NULL; - tal_wally_end(tal_steal(ctx, wtx)); + tal_wally_end_onto(ctx, wtx, struct wally_tx); return wtx; } @@ -672,7 +672,7 @@ struct wally_psbt *psbt_from_b64(const tal_t *ctx, tal_add_destructor(psbt, psbt_destroy); else psbt = NULL; - tal_wally_end(tal_steal(ctx, psbt)); + tal_wally_end_onto(ctx, psbt, struct wally_psbt); return psbt; } @@ -685,7 +685,7 @@ char *psbt_to_b64(const tal_t *ctx, const struct wally_psbt *psbt) tal_wally_start(); ret = wally_psbt_to_base64(psbt, 0, &serialized_psbt); assert(ret == WALLY_OK); - tal_wally_end(tal_steal(ctx, serialized_psbt)); + tal_wally_end_onto(ctx, serialized_psbt, char); return serialized_psbt; } @@ -723,7 +723,7 @@ struct wally_psbt *psbt_from_bytes(const tal_t *ctx, const u8 *bytes, tal_add_destructor(psbt, psbt_destroy); else psbt = NULL; - tal_wally_end(tal_steal(ctx, psbt)); + tal_wally_end_onto(ctx, psbt, struct wally_psbt); return psbt; } @@ -798,7 +798,7 @@ void psbt_txid(const tal_t *ctx, wally_tx_set_input_script(tx, i, script, tal_bytelen(script)); } } - tal_wally_end(tal_steal(ctx, tx)); + tal_wally_end_onto(ctx, tx, struct wally_tx); wally_txid(tx, txid); if (wtx) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 15c869862..29d0ed8ab 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -54,7 +54,7 @@ struct wally_tx_output *wally_tx_output(const tal_t *ctx, } done: - tal_wally_end(tal_steal(ctx, output)); + tal_wally_end_onto(ctx, output, struct wally_tx_output); return output; } @@ -520,7 +520,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_add_destructor(tx, bitcoin_tx_destroy); - tal_wally_end(tal_steal(tx, tx->wtx)); + tal_wally_end_onto(tx, tx->wtx, struct wally_tx); tx->chainparams = chainparams; tx->psbt = new_psbt(tx, tx->wtx); @@ -548,7 +548,7 @@ struct bitcoin_tx *bitcoin_tx_with_psbt(const tal_t *ctx, struct wally_psbt *psb tal_wally_start(); if (wally_tx_clone_alloc(psbt->tx, 0, &tx->wtx) != WALLY_OK) tx->wtx = NULL; - tal_wally_end(tal_steal(tx, tx->wtx)); + tal_wally_end_onto(tx, tx->wtx, struct wally_tx); if (!tx->wtx) return tal_free(tx); } @@ -574,7 +574,7 @@ static struct wally_tx *pull_wtx(const tal_t *ctx, fromwire_fail(cursor, max); wtx = tal_free(wtx); } - tal_wally_end(tal_steal(ctx, wtx)); + tal_wally_end_onto(ctx, wtx, struct wally_tx); if (wtx) { size_t wsize; diff --git a/bitcoin/tx_parts.c b/bitcoin/tx_parts.c index e61a388be..970e489f8 100644 --- a/bitcoin/tx_parts.c +++ b/bitcoin/tx_parts.c @@ -154,7 +154,7 @@ fromwire_wally_tx_witness_stack(const tal_t *ctx, tal_add_destructor(ws, destroy_wally_tx_witness_stack); out: - tal_wally_end(tal_steal(ctx, ws)); + tal_wally_end_onto(ctx, ws, struct wally_tx_witness_stack); return ws; } @@ -252,7 +252,7 @@ static struct wally_tx_input *fromwire_wally_tx_input(const tal_t *ctx, tal_add_destructor(in, destroy_wally_tx_input); } - tal_wally_end(tal_steal(ctx, in)); + tal_wally_end_onto(ctx, in, struct wally_tx_input); return in; } @@ -310,7 +310,7 @@ static struct wally_tx_output *fromwire_wally_tx_output(const tal_t *ctx, } else { tal_add_destructor(out, destroy_wally_tx_output); } - tal_wally_end(tal_steal(ctx, out)); + tal_wally_end_onto(ctx, out, struct wally_tx_output); return out; } diff --git a/common/utils.c b/common/utils.c index a11598fb1..c3c5b0fb9 100644 --- a/common/utils.c +++ b/common/utils.c @@ -34,20 +34,29 @@ void tal_wally_end(const tal_t *parent) { tal_t *p; while ((p = tal_first(wally_tal_ctx)) != NULL) { - if (p != parent) { + /* Refuse to make a loop! */ + assert(p != parent); #if DEVELOPER - /* Don't steal backtrace from wally_tal_ctx! */ - if (tal_name(p) && streq(tal_name(p), "backtrace")) { - tal_free(p); - continue; - } -#endif /* DEVELOPER */ - tal_steal(parent, p); + /* Don't steal backtrace from wally_tal_ctx! */ + if (tal_name(p) && streq(tal_name(p), "backtrace")) { + tal_free(p); + continue; } +#endif /* DEVELOPER */ + tal_steal(parent, p); } wally_tal_ctx = tal_free(wally_tal_ctx); } +void tal_wally_end_onto_(const tal_t *parent, + tal_t *from_wally, + const char *from_wally_name) +{ + if (from_wally) + tal_set_name_(from_wally, from_wally_name, 1); + tal_wally_end(tal_steal(parent, from_wally)); +} + #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 69314f8c2..dcfa111cb 100644 --- a/common/utils.h +++ b/common/utils.h @@ -105,10 +105,21 @@ void clean_tmpctx(void); /* Call this before any libwally function which allocates. */ void tal_wally_start(void); -/* Then call this to reparent everything onto this parent (which must - * have been tal_steal() if it was allocated by libwally here) */ + +/* Then call this to reparent everything onto this parent */ void tal_wally_end(const tal_t *parent); +/* ... or this if you want to reparent onto something which is + * allocated by libwally here. Fixes up this from_wally obj to have a + * proper tal_name, too! */ +#define tal_wally_end_onto(parent, from_wally, type) \ + tal_wally_end_onto_((parent), \ + (from_wally) + 0*sizeof((from_wally) == (type *)0), \ + stringify(type)) +void tal_wally_end_onto_(const tal_t *parent, + tal_t *from_wally, + const char *from_wally_name); + /* Define sha256_eq. */ STRUCTEQ_DEF(sha256, 0, u); diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index fb7df13c7..3aa4f98ad 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -1470,7 +1470,9 @@ static void send_funding_tx(struct channel *channel, wally_tx_clone_alloc(wtx, 0, cast_const2(struct wally_tx **, &cs->wtx)); - tal_wally_end(tal_steal(cs, cs->wtx)); + tal_wally_end_onto(cs, + cast_const(struct wally_tx *, + cs->wtx), struct wally_tx); } wally_txid(wtx, &txid); @@ -2405,7 +2407,7 @@ json_openchannel_signed(struct command *cmd, /* Make memleak happy, (otherwise cleaned up with `cmd`) */ tal_free(psbt); - tal_wally_end(tal_steal(inflight, inflight->funding_psbt)); + tal_wally_end_onto(inflight, inflight->funding_psbt, struct wally_psbt); /* Update the PSBT on disk */ wallet_inflight_save(cmd->ld->wallet, inflight); diff --git a/plugins/spender/multifundchannel.c b/plugins/spender/multifundchannel.c index e2ada2bd1..758524e1f 100644 --- a/plugins/spender/multifundchannel.c +++ b/plugins/spender/multifundchannel.c @@ -179,7 +179,7 @@ mfc_cleanup_psbt(struct command *cmd, tal_wally_start(); if (wally_psbt_clone_alloc(psbt, 0, &pruned_psbt) != WALLY_OK) abort(); - tal_wally_end(tal_steal(NULL, pruned_psbt)); + tal_wally_end_onto(NULL, pruned_psbt, struct wally_psbt); for (size_t i = pruned_psbt->num_inputs - 1; i < pruned_psbt->num_inputs; diff --git a/plugins/spender/openchannel.c b/plugins/spender/openchannel.c index 501f28ca7..3202eb5d3 100644 --- a/plugins/spender/openchannel.c +++ b/plugins/spender/openchannel.c @@ -76,7 +76,7 @@ static bool update_parent_psbt(const tal_t *ctx, tal_wally_start(); if (wally_psbt_clone_alloc(*parent_psbt, 0, &clone) != WALLY_OK) abort(); - tal_wally_end(tal_steal(ctx, clone)); + tal_wally_end_onto(ctx, clone, struct wally_psbt); /* This makes it such that we can reparent/steal added * inputs/outputs without impacting the 'original'. We @@ -261,7 +261,7 @@ static bool update_node_psbt(const tal_t *ctx, /* Only failure is alloc */ if (wally_psbt_clone_alloc(parent_psbt, 0, &clone) != WALLY_OK) abort(); - tal_wally_end(tal_steal(ctx, clone)); + tal_wally_end_onto(ctx, clone, struct wally_psbt); /* For every peer's input/output, flip the serial id * on the clone. They should all be present. */ @@ -958,7 +958,7 @@ openchannel_init_ok(struct command *cmd, * logic in `perform_openchannel_update` the same. */ tal_wally_start(); wally_psbt_clone_alloc(dest->updated_psbt, 0, &dest->psbt); - tal_wally_end(tal_steal(mfc, dest->updated_psbt)); + tal_wally_end_onto(mfc, dest->updated_psbt, struct wally_psbt); return openchannel_init_done(dest); } @@ -998,7 +998,7 @@ openchannel_init_dest(struct multifundchannel_destination *dest) /* Copy the original parent down */ tal_wally_start(); wally_psbt_clone_alloc(mfc->psbt, 0, &dest->psbt); - tal_wally_end(tal_steal(mfc, dest->psbt)); + tal_wally_end_onto(mfc, dest->psbt, struct wally_psbt); json_add_psbt(req->js, "initialpsbt", dest->psbt); if (mfc->cmtmt_feerate_str)