← Watch feed
htlc_wire: fix crash when adding an HTLC
Public commit record
78/100 · Adequate
What the developer wrote
Authored by Lagrang3
htlc_wire: fix crash when adding an HTLC
In line channeld/channeld_wiregen.c:832 `*added+i` is not a tal object hence
the instruction in common/htlc_wire.c:200 `tal_arr(ctx, struct tlv_field, 0);` crashes CLN.
This is fixed by stating that added_htlc is a a varsize_type.
Logs:
2025-08-16T02:25:28.640Z **BROKEN** lightningd: FATAL SIGNAL 6 (version v25.05-200-g79b959b)V
...
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:95 (call_error) 0x54f6bc
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:169 (check_bounds) 0x54f75a
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:178 (to_tal_hdr) 0x54f782
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:193 (to_tal_hdr_or_null) 0x54f7c7
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:471 (tal_alloc_) 0x54ffe4
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/tal/tal.c:517 (tal_alloc_arr_) 0x5500c4
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: common/htlc_wire.c:200 (fromwire_len_and_tlvstream) 0x48d63d
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: common/htlc_wire.c:234 (fromwire_added_htlc) 0x48dd23
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: channeld/channeld_wiregen.c:832 (fromwire_channeld_got_commitsig) 0x4c61fa
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: lightningd/peer_htlcs.c:2377 (peer_got_commitsig) 0x4549cb
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: lightningd/channel_control.c:1552 (channel_msg) 0x4140fe
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: lightningd/subd.c:560 (sd_msg_read) 0x461513
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:60 (next_plan) 0x544885
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:422 (do_plan) 0x544cea
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:439 (io_ready) 0x544d9d
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: ccan/ccan/io/poll.c:455 (io_loop) 0x54665d
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: lightningd/io_loop_with_timers.c:22 (io_loop_with_timers) 0x42d220
2025-08-16T02:25:28.640Z **BROKEN** lightningd: backtrace: lightningd/lightningd.c:1487 (main) 0x43280f
gdb inspection:
830 *added = num_added ? tal_arr(ctx, struct added_htlc, num_added) : NULL;
831 for (size_t i = 0; i < num_added; i++)
832 fromwire_added_htlc(&cursor, &plen, *added + i);
(gdb) p i
$3 = 1
Changelog-None: crash introduced this release.
Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
[ Added test, removed Changelog --RR ]
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification
What changed, and why it matters
This commit fixes a crash bug in Core Lightning (CLN) that occurs when a peer adds more than one HTLC (a payment channel transaction) in a single commitment round. The root cause was a memory-management mistake: the generated wire-deserialization code treated an array of 'added_htlc' records as if each element were an independently allocated memory object, but only the array's head was. When the second element was processed, the code tried to allocate child objects against a non-TAL pointer, triggering an assertion/crash. The fix changes 'added_htlc' to a varsize pointer type so each record is allocated separately. The commit message and a test name indicate this could be hit in normal operation and caused a FATAL SIGNAL 6 (abort).
Recommended action
Upgrade to a CLN release containing this commit. Nodes running the affected release (v25.05-200-g79b959b and nearby commits) are at risk of remote-triggered crash when receiving a commitsig with multiple added HTLCs. Until patched, operators should monitor for unexpected daemon restarts and avoid untrusted high-volume channels if possible.
Security signals we found
01
Denial-of-service vector: remote peer can crash the local lightningd/channeld by adding multiple HTLCs in one commitment signature message
02
Memory allocator invariant violation: passing interior array pointer as tal context
03
FATAL SIGNAL 6 (SIGABRT) backtrace provided in commit message
04
Test test_htlc_tlv_crash was marked xfail and is now enabled, confirming reproducible crash
05
Fix touches generated wire code and declares added_htlc as varsize_type
Evidence from the diff
The bug is in the wire marshalling layer. channeld/channeld_wiregen.c:830-832 allocates *added as a tal_arr of struct added_htlc, then calls fromwire_added_htlc(&cursor, &plen, added + i). fromwire_added_htlc internally calls tal_arr(ctx, struct tlv_field, 0) using the passed added pointer as ctx. For i > 0, *added + i is an interior pointer into the tal array, not a tal object itself, so tal_arr() aborts in check_bounds/to_tal_hdr. The fix makes added_htlc a varsize_type in tools/generate-wire.py, which causes generated code to deserialize into an array of pointers (struct added_htlc **added) and fromwire_added_htlc now allocates its own struct added_htlc with a proper tal parent. Callers in channeld.c, peer_htlcs.c, and the wallet test stub are updated to use const struct added_htlc **. A previously xfail test, test_htlc_tlv_crash, is re-enabled.
Changed components
common/htlc_wire.ccommon/htlc_wire.hchanneld/channeld.clightningd/peer_htlcs.ctools/generate-wire.pychanneld/channeld_wiregen.c (generated)wallet/test/run-wallet.cInspect captured patch +27 / −23
diff --git a/channeld/channeld.c b/channeld/channeld.c
index fe7f7707..eb1b7a50 100644
--- a/channeld/channeld.c
+++ b/channeld/channeld.c
@@ -1571,28 +1571,30 @@ static void marshall_htlc_info(const tal_t *ctx,
struct changed_htlc **changed,
struct fulfilled_htlc **fulfilled,
const struct failed_htlc ***failed,
- struct added_htlc **added)
+ const struct added_htlc ***added)
{
*changed = tal_arr(ctx, struct changed_htlc, 0);
- *added = tal_arr(ctx, struct added_htlc, 0);
+ *added = tal_arr(ctx, const struct added_htlc *, 0);
*failed = tal_arr(ctx, const struct failed_htlc *, 0);
*fulfilled = tal_arr(ctx, struct fulfilled_htlc, 0);
for (size_t i = 0; i < tal_count(changed_htlcs); i++) {
const struct htlc *htlc = changed_htlcs[i];
if (htlc->state == RCVD_ADD_COMMIT) {
- struct added_htlc a;
+ struct added_htlc *a = tal(*added, struct added_htlc);
- a.id = htlc->id;
- a.amount = htlc->amount;
- a.payment_hash = htlc->rhash;
- a.cltv_expiry = abs_locktime_to_blocks(&htlc->expiry);
- memcpy(a.onion_routing_packet,
+ a->id = htlc->id;
+ a->amount = htlc->amount;
+ a->payment_hash = htlc->rhash;
+ a->cltv_expiry = abs_locktime_to_blocks(&htlc->expiry);
+ memcpy(a->onion_routing_packet,
htlc->routing,
- sizeof(a.onion_routing_packet));
- a.path_key = htlc->path_key;
- a.extra_tlvs = htlc->extra_tlvs;
- a.fail_immediate = htlc->fail_immediate;
+ sizeof(a->onion_routing_packet));
+ /* Note: we assume htlc's lifetime is greater than ours,
+ * so we just share pointers and don't bother copying */
+ a->path_key = htlc->path_key;
+ a->extra_tlvs = htlc->extra_tlvs;
+ a->fail_immediate = htlc->fail_immediate;
tal_arr_expand(added, a);
} else if (htlc->state == RCVD_REMOVE_COMMIT) {
if (htlc->r) {
@@ -1629,7 +1631,7 @@ static void send_revocation(struct peer *peer,
struct changed_htlc *changed;
struct fulfilled_htlc *fulfilled;
const struct failed_htlc **failed;
- struct added_htlc *added;
+ const struct added_htlc **added;
const u8 *msg;
const u8 *msg_for_master;
diff --git a/common/htlc_wire.c b/common/htlc_wire.c
index 49c8211d..bb17e738 100644
--- a/common/htlc_wire.c
+++ b/common/htlc_wire.c
@@ -216,9 +216,10 @@ static struct tlv_field *fromwire_len_and_tlvstream(const tal_t *ctx,
return tlvs;
}
-void fromwire_added_htlc(const u8 **cursor, size_t *max,
- struct added_htlc *added)
+struct added_htlc *fromwire_added_htlc(const tal_t *ctx, const u8 **cursor,
+ size_t *max)
{
+ struct added_htlc *added = tal(ctx, struct added_htlc);
added->id = fromwire_u64(cursor, max);
added->amount = fromwire_amount_msat(cursor, max);
fromwire_sha256(cursor, max, &added->payment_hash);
@@ -235,6 +236,7 @@ void fromwire_added_htlc(const u8 **cursor, size_t *max,
} else
added->extra_tlvs = NULL;
added->fail_immediate = fromwire_bool(cursor, max);
+ return added;
}
struct existing_htlc *fromwire_existing_htlc(const tal_t *ctx,
diff --git a/common/htlc_wire.h b/common/htlc_wire.h
index 5cc94a98..2ef90e9e 100644
--- a/common/htlc_wire.h
+++ b/common/htlc_wire.h
@@ -86,8 +86,8 @@ void towire_changed_htlc(u8 **pptr, const struct changed_htlc *changed);
void towire_side(u8 **pptr, const enum side side);
void towire_shachain(u8 **pptr, const struct shachain *shachain);
-void fromwire_added_htlc(const u8 **cursor, size_t *max,
- struct added_htlc *added);
+struct added_htlc *fromwire_added_htlc(const tal_t *ctx, const u8 **cursor,
+ size_t *max);
struct existing_htlc *fromwire_existing_htlc(const tal_t *ctx,
const u8 **cursor, size_t *max);
void fromwire_fulfilled_htlc(const u8 **cursor, size_t *max,
diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
index 454eb8dc..27c3a475 100644
--- a/lightningd/peer_htlcs.c
+++ b/lightningd/peer_htlcs.c
@@ -2309,7 +2309,7 @@ static bool channel_added_their_htlc(struct channel *channel,
/* The peer doesn't tell us this separately, but logically it's a separate
* step to receiving commitsig */
static bool peer_sending_revocation(struct channel *channel,
- struct added_htlc *added,
+ struct added_htlc **added,
struct fulfilled_htlc *fulfilled,
struct failed_htlc **failed,
struct changed_htlc *changed)
@@ -2317,7 +2317,7 @@ static bool peer_sending_revocation(struct channel *channel,
size_t i;
for (i = 0; i < tal_count(added); i++) {
- if (!update_in_htlc(channel, added[i].id, SENT_ADD_REVOCATION))
+ if (!update_in_htlc(channel, added[i]->id, SENT_ADD_REVOCATION))
return false;
}
for (i = 0; i < tal_count(fulfilled); i++) {
@@ -2364,7 +2364,7 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg)
struct fee_states *fee_states;
struct height_states *blockheight_states;
struct bitcoin_signature commit_sig, *htlc_sigs;
- struct added_htlc *added;
+ struct added_htlc **added;
struct fulfilled_htlc *fulfilled;
struct failed_htlc **failed;
struct changed_htlc *changed;
@@ -2439,7 +2439,7 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg)
/* New HTLCs */
for (i = 0; i < tal_count(added); i++) {
- if (!channel_added_their_htlc(channel, &added[i]))
+ if (!channel_added_their_htlc(channel, added[i]))
return;
}
diff --git a/tests/test_pay.py b/tests/test_pay.py
index 1d3b9353..151d5603 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -7016,7 +7016,6 @@ def test_sendonion_sendpay(node_factory, bitcoind):
assert invoice["amount_received_msat"] == Millisatoshi(total_amount)
-@pytest.mark.xfail(strict=True)
def test_htlc_tlv_crash(node_factory):
"""Marshalling code treated an array of htlc_added as if they were tal objects, but only the head is a tal object so if we have more than one, BOOM!"""
plugin = os.path.join(os.path.dirname(__file__), 'plugins/htlc_accepted-customtlv.py')
diff --git a/tools/generate-wire.py b/tools/generate-wire.py
index dd6c7424..e78d68c5 100755
--- a/tools/generate-wire.py
+++ b/tools/generate-wire.py
@@ -230,6 +230,7 @@ class Type(FieldSet):
'gossip_getnodes_entry',
'gossip_getchannels_entry',
'failed_htlc',
+ 'added_htlc',
'existing_htlc',
'inflight',
'hsm_utxo',
diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c
index 92ec32bc..c1cd3b06 100644
--- a/wallet/test/run-wallet.c
+++ b/wallet/test/run-wallet.c
@@ -300,7 +300,7 @@ struct channel_type *fromwire_channel_type(const tal_t *ctx UNNEEDED, const u8 *
bool fromwire_channeld_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEEDED)
{ fprintf(stderr, "fromwire_channeld_dev_memleak_reply called!\n"); abort(); }
/* Generated stub for fromwire_channeld_got_commitsig */
-bool fromwire_channeld_got_commitsig(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *commitnum UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct bitcoin_signature *signature UNNEEDED, struct bitcoin_signature **htlc_signature UNNEEDED, struct added_htlc **added UNNEEDED, struct fulfilled_htlc **fulfilled UNNEEDED, struct failed_htlc ***failed UNNEEDED, struct changed_htlc **changed UNNEEDED, struct bitcoin_tx **tx UNNEEDED, struct commitsig ***inflight_commitsigs UNNEEDED)
+bool fromwire_channeld_got_commitsig(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *commitnum UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct bitcoin_signature *signature UNNEEDED, struct bitcoin_signature **htlc_signature UNNEEDED, struct added_htlc ***added UNNEEDED, struct fulfilled_htlc **fulfilled UNNEEDED, struct failed_htlc ***failed UNNEEDED, struct changed_htlc **changed UNNEEDED, struct bitcoin_tx **tx UNNEEDED, struct commitsig ***inflight_commitsigs UNNEEDED)
{ fprintf(stderr, "fromwire_channeld_got_commitsig called!\n"); abort(); }
/* Generated stub for fromwire_channeld_got_revoke */
bool fromwire_channeld_got_revoke(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *revokenum UNNEEDED, struct secret *per_commitment_secret UNNEEDED, struct pubkey *next_per_commit_point UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct changed_htlc **changed UNNEEDED, struct penalty_base **pbase UNNEEDED, struct bitcoin_tx **penalty_tx UNNEEDED)
Risk score
Our methodology →Why this scored 67/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.