Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close PSBT leaks found by Christian, fixes to detect them in future #4071

Merged
merged 15 commits into from
Sep 23, 2020
Merged

Close PSBT leaks found by Christian, fixes to detect them in future #4071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3b0c246
bitcoin/psbt: more const pointers.
rustyrussell Sep 21, 2020
32f035d
bitcoin/psbt: attach destructors to wally allocations to avoid leaks.
rustyrussell Sep 21, 2020
ddcadae
bitcoin/tx: trivial cleanups.
rustyrussell Sep 21, 2020
46ea7cb
bitcoin/psbt: psbt_txid needs a tal ctx.
rustyrussell Sep 21, 2020
596298a
bitcoin/psbt: wallt_tx_output needs a tal ctx.
rustyrussell Sep 21, 2020
4b7199c
bitcoin/psbt: psbt_finalize needs a tal ctx.
rustyrussell Sep 21, 2020
ea2681e
bitcoin/psbt: psbt_input_add_unknown/psbt_output_add_unknown needs a …
rustyrussell Sep 22, 2020
88e3c14
common: add tal_gather_wally() function to reparent libwally objs.
rustyrussell Sep 22, 2020
40ee7fc
bitcoin: use tal_gather_wally() so we don't leave unattached allocati…
rustyrussell Sep 23, 2020
8a9d7d5
common: enforce that we have collected all wally allocations.
rustyrussell Sep 23, 2020
b8f6a3c
memleak: make "_notleak" names less powerful.
rustyrussell Sep 23, 2020
2fa1986
common: don't suppress leak detection for libwally allocations.
rustyrussell Sep 23, 2020
7709990
pytest: don't test for memleaks under valgrind.
rustyrussell Sep 23, 2020
20a2476
common/memleak: simplify and document API.
rustyrussell Sep 23, 2020
d17a3c5
topo: Do not keep txids in memory indefinitely
cdecker Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 65 additions & 24 deletions bitcoin/psbt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any harm in calling it unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No harm, but we have a debug check for the moment that we don't call tal_gather_wally unless there's something to gather. Found some places where I was being overzealous.

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)
Expand All @@ -44,11 +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;
}

Expand Down Expand Up @@ -83,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)
Expand All @@ -103,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];
}

Expand Down Expand Up @@ -140,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 */
Expand All @@ -151,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];
Expand All @@ -169,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];
}

Expand All @@ -177,7 +189,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);
Expand All @@ -189,7 +201,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);
Expand Down Expand Up @@ -230,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,
Expand All @@ -243,16 +256,19 @@ 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,
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);
Expand All @@ -265,7 +281,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,
Expand All @@ -274,13 +290,15 @@ 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);
tal_gather_wally(psbt);
}

void psbt_input_set_witscript(struct wally_psbt *psbt, size_t in, const u8 *wscript)
Expand All @@ -291,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,
Expand All @@ -306,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)
Expand Down Expand Up @@ -350,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];
Expand All @@ -359,8 +380,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++) {
Expand Down Expand Up @@ -388,7 +409,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;
Expand All @@ -408,7 +429,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;
Expand Down Expand Up @@ -462,7 +483,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)
Expand All @@ -472,6 +494,7 @@ void psbt_input_add_unknown(struct wally_psbt_input *in,
(unsigned char *) memcheck(value, value_len), value_len)
!= WALLY_OK)
abort();
tal_gather_wally(ctx);
}

void *psbt_get_unknown(const struct wally_map *map,
Expand Down Expand Up @@ -504,7 +527,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)
Expand All @@ -514,9 +538,17 @@ void psbt_output_add_unknown(struct wally_psbt_output *out,
(unsigned char *) memcheck(value, value_len), value_len)
!= WALLY_OK)
abort();
tal_gather_wally(ctx);
}

struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place)
/* 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(const tal_t *ctx,
struct wally_psbt *psbt, bool finalize_in_place)
{
struct wally_psbt *tmppsbt;
struct wally_tx *wtx;
Expand All @@ -527,6 +559,8 @@ 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;
/* Explicitly freed below */
tal_steal(NULL, tmppsbt);
} else
tmppsbt = cast_const(struct wally_psbt *, psbt);

Expand Down Expand Up @@ -566,6 +600,7 @@ struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place)
input->witness_script,
input->witness_script_len);
input->final_witness = stack;
tal_gather_wally(tmppsbt);
}

if (wally_psbt_finalize(tmppsbt) != WALLY_OK) {
Expand All @@ -574,8 +609,14 @@ struct wally_tx *psbt_finalize(struct wally_psbt *psbt, bool finalize_in_place)
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_gather_wally(tal_steal(ctx, wtx));
tal_add_destructor(wtx, wally_tx_destroy);
if (!finalize_in_place)
wally_psbt_free(tmppsbt);
return wtx;
Expand All @@ -597,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;
}
Expand All @@ -614,8 +655,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,
Expand Down Expand Up @@ -649,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;
}
Expand Down Expand Up @@ -696,7 +735,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;
Expand All @@ -719,6 +759,7 @@ void psbt_txid(const struct wally_psbt *psbt, struct bitcoin_txid *txid,
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)
Expand Down
Loading