bkpr: take, don't steal in new_channel_event.
What the developer wrote
Authored by Rusty Russell
bkpr: take, don't steal in new_channel_event.
Cleaner (I'm about to hand it a sha256 on the stack).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This is a small cleanup change in the bookkeeping (bkpr) plugin. It changes how a payment identifier (a 32-byte hash value) is passed into a function: instead of 'stealing' ownership of the caller's copy, it now 'takes' a read-only pointer and makes its own duplicate if needed. The commit message says this is to make it easier to pass a temporary value created on the stack. This is defensive coding and reduces the risk of memory-management mistakes, but it does not by itself fix a known exploitable bug.
No urgent action. Treat as routine defensive cleanup. Reviewers of the follow-up change (passing stack sha256) should verify all callers are updated and that no caller still expects the old steal semantics.
Security signals we found
Memory ownership semantics changed from STEALS to TAKES
Use of tal_dup_or_null instead of tal_steal prevents use-after-free or double-free risks from callers
Commit author notes this is cleanup in preparation for passing a stack-allocated sha256
No explicit security bug or CVE referenced in commit or supplied materials
Evidence from the diff
The patch converts new_channel_event’s payment_id parameter from a mutable ‘STEALS’ pointer to a const ‘TAKES’ pointer. Internally, tal_steal(ev, payment_id) is replaced with tal_dup_or_null(ev, struct sha256, payment_id). This means callers no longer need to allocate and transfer ownership of a sha256; they can pass stack-allocated or borrowed values. The change is in the bkpr plugin only (channel_event.c/h and a test stub). It is a refactor/cleanup with a defensive-security side effect: it removes a footgun where callers might pass a pointer they still need or pass a stack pointer that must not be stolen.
Changed components
plugins/bkpr/channel_event.cplugins/bkpr/channel_event.hplugins/bkpr/test/run-sql.cInspect captured patch +4 / −4
diff --git a/plugins/bkpr/channel_event.c b/plugins/bkpr/channel_event.c
index 59a051b7..ded346cf 100644
--- a/plugins/bkpr/channel_event.c
+++ b/plugins/bkpr/channel_event.c
@@ -14,7 +14,7 @@ struct channel_event *new_channel_event(const tal_t *ctx,
struct amount_msat credit,
struct amount_msat debit,
struct amount_msat fees,
- struct sha256 *payment_id STEALS,
+ const struct sha256 *payment_id TAKES,
u32 part_id,
u64 timestamp)
{
@@ -24,7 +24,7 @@ struct channel_event *new_channel_event(const tal_t *ctx,
ev->credit = credit;
ev->debit = debit;
ev->fees = fees;
- ev->payment_id = tal_steal(ev, payment_id);
+ ev->payment_id = tal_dup_or_null(ev, struct sha256, payment_id);
ev->part_id = part_id;
ev->timestamp = timestamp;
diff --git a/plugins/bkpr/channel_event.h b/plugins/bkpr/channel_event.h
index fc39875f..08cd357e 100644
--- a/plugins/bkpr/channel_event.h
+++ b/plugins/bkpr/channel_event.h
@@ -45,7 +45,7 @@ struct channel_event *new_channel_event(const tal_t *ctx,
struct amount_msat credit,
struct amount_msat debit,
struct amount_msat fees,
- struct sha256 *payment_id STEALS,
+ const struct sha256 *payment_id TAKES,
u32 part_id,
u64 timestamp);
diff --git a/plugins/bkpr/test/run-sql.c b/plugins/bkpr/test/run-sql.c
index cd2a4b09..3d497492 100644
--- a/plugins/bkpr/test/run-sql.c
+++ b/plugins/bkpr/test/run-sql.c
@@ -132,7 +132,7 @@ struct channel_event *new_channel_event(const tal_t *ctx UNNEEDED,
struct amount_msat credit UNNEEDED,
struct amount_msat debit UNNEEDED,
struct amount_msat fees UNNEEDED,
- struct sha256 *payment_id STEALS UNNEEDED,
+ const struct sha256 *payment_id TAKES UNNEEDED,
u32 part_id UNNEEDED,
u64 timestamp UNNEEDED)
{ fprintf(stderr, "new_channel_event called!\n"); abort(); }
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.