common: handle taken() extra_tlvs in new_existing_htlc properly.
What the developer wrote
Authored by Rusty Russell
common: handle taken() extra_tlvs in new_existing_htlc properly.
Reported-by: Christian Decker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This patch fixes a memory-management bug when handling extra TLV (type-length-value) data attached to Lightning Network HTLCs (payment hops). The bug involved the 'taken()' memory ownership marker: when the caller had marked the extra_tlvs array to be 'taken' (transferred), the old code would still make a full copy and deep-copy of every value buffer, which is unnecessary and could lead to use-after-free or double-free problems because the original array's value buffers were not re-parented correctly. The patch introduces a helper that checks whether the array is already 'taken' and, if so, avoids the redundant deep copy, ensuring the value buffers are attached to the correct parent object.
Treat as a low-to-moderate reliability/security fix. Apply the patch and run the project's memory-sanitizer/valgrind tests, especially any exercising HTLC creation with extra_tlvs. No immediate emergency response is indicated, but node operators should upgrade in due course to avoid potential memory corruption during payment forwarding.
Security signals we found
Memory ownership/lifetime bug in TLV array duplication
Incorrect handling of TAKES/taken() semantics for extra_tlvs
Potential use-after-free or double-free in HTLC metadata handling
Fix reported by project contributor Christian Decker
Patch is defensive/refactoring with explicit memory-ownership semantics
Evidence from the diff
The change centralizes duplication of struct tlv_field arrays into a new helper tlv_field_arr_dup() in common/htlc_wire.c. The helper uses is_taken(arr) to decide whether the input array’s buffers are being transferred. If not taken, it performs tal_dup_talarr() on the array and then re-parents each value buffer with tal_dup_talarr(ret, u8, ret[i].value). If taken, it skips the per-value copy. new_existing_htlc(), new_htlc_in(), and new_htlc_out() are updated to use this helper. The previous code always deep-copied values, which is incorrect when the caller intended to transfer ownership via TAKES, because the original value pointers could later be freed by the caller while the duplicated array still references them, or vice versa, leading to memory corruption.
Changed components
common/htlc_wire.ccommon/htlc_wire.hlightningd/htlc_end.cHTLC in/out construction pathsexisting_htlc construction pathInspect captured patch +31 / −26
diff --git a/common/htlc_wire.c b/common/htlc_wire.c
index 10893156..b50e7c29 100644
--- a/common/htlc_wire.c
+++ b/common/htlc_wire.c
@@ -25,6 +25,23 @@ static struct failed_htlc *failed_htlc_dup(const tal_t *ctx,
return newf;
}
+/* Helper to duplicate an array of tlv_field (vs an array of tlv_field *) */
+struct tlv_field *tlv_field_arr_dup(const tal_t *ctx,
+ const struct tlv_field *arr TAKES)
+{
+ struct tlv_field *ret;
+ bool needs_copy = !is_taken(arr);
+
+ ret = tal_dup_talarr(ctx, struct tlv_field, arr);
+ if (needs_copy) {
+ for (size_t i = 0; i < tal_count(ret); i++) {
+ /* We need to attach the value to the correct parent */
+ ret[i].value = tal_dup_talarr(ret, u8, ret[i].value);
+ }
+ }
+ return ret;
+}
+
struct existing_htlc *new_existing_htlc(const tal_t *ctx,
u64 id,
enum htlc_state state,
@@ -53,17 +70,10 @@ struct existing_htlc *new_existing_htlc(const tal_t *ctx,
existing->failed = failed_htlc_dup(existing, failed);
else
existing->failed = NULL;
- if (extra_tlvs) {
- existing->extra_tlvs = tal_dup_talarr(existing, struct tlv_field, extra_tlvs);
- for (size_t i = 0; i < tal_count(extra_tlvs); i++) {
- /* We need to attach the value to the correct parent */
- existing->extra_tlvs[i].value
- = tal_dup_talarr(existing, u8,
- existing->extra_tlvs[i].value);
- }
- } else {
+ if (extra_tlvs)
+ existing->extra_tlvs = tlv_field_arr_dup(existing, extra_tlvs);
+ else
existing->extra_tlvs = NULL;
- }
return existing;
}
diff --git a/common/htlc_wire.h b/common/htlc_wire.h
index c50fece3..5cc94a98 100644
--- a/common/htlc_wire.h
+++ b/common/htlc_wire.h
@@ -62,6 +62,10 @@ struct changed_htlc {
u64 id;
};
+/* Helper to duplicate an array of tlv_field (vs an array of tlv_field *) */
+struct tlv_field *tlv_field_arr_dup(const tal_t *ctx,
+ const struct tlv_field *arr TAKES);
+
struct existing_htlc *new_existing_htlc(const tal_t *ctx,
u64 id,
enum htlc_state state,
diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c
index b040f3b6..91d66b0c 100644
--- a/lightningd/htlc_end.c
+++ b/lightningd/htlc_end.c
@@ -3,6 +3,7 @@
#include <ccan/crypto/siphash24/siphash24.h>
#include <ccan/tal/str/str.h>
#include <common/htlc.h>
+#include <common/htlc_wire.h>
#include <common/pseudorand.h>
#include <lightningd/htlc_end.h>
#include <lightningd/log.h>
@@ -148,15 +149,10 @@ struct htlc_in *new_htlc_in(const tal_t *ctx,
hin->path_key = tal_dup_or_null(hin, struct pubkey, path_key);
memcpy(hin->onion_routing_packet, onion_routing_packet,
sizeof(hin->onion_routing_packet));
- if (extra_tlvs) {
- hin->extra_tlvs = tal_dup_talarr(hin, struct tlv_field, extra_tlvs);
- for (size_t i = 0; i < tal_count(extra_tlvs); i++) {
- /* We need to attach the value to the correct parent */
- hin->extra_tlvs[i].value = tal_dup_talarr(hin, u8, hin->extra_tlvs[i].value);
- }
- } else {
+ if (extra_tlvs)
+ hin->extra_tlvs = tlv_field_arr_dup(hin, extra_tlvs);
+ else
hin->extra_tlvs = NULL;
- }
hin->hstate = RCVD_ADD_COMMIT;
hin->badonion = 0;
@@ -304,15 +300,10 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
hout->path_key = tal_dup_or_null(hout, struct pubkey, path_key);
- if (extra_tlvs) {
- hout->extra_tlvs = tal_dup_talarr(hout, struct tlv_field, extra_tlvs);
- for (size_t i = 0; i < tal_count(extra_tlvs); i++) {
- /* We need to attach the value to the correct parent */
- hout->extra_tlvs[i].value = tal_dup_talarr(hout, u8, hout->extra_tlvs[i].value);
- }
- } else {
+ if (extra_tlvs)
+ hout->extra_tlvs = tlv_field_arr_dup(hout, extra_tlvs);
+ else
hout->extra_tlvs = NULL;
- }
hout->am_origin = am_origin;
if (am_origin) {
Why this scored 42/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.