Skip to content

bitcoin: don't dereference a NULL transaction from a truncated block - #9485

Open
kwsantiago wants to merge 2 commits into
ElementsProject:masterfrom
privkeyio:fix/null-tx-truncated-block
Open

kwsantiago wants to merge 2 commits into
ElementsProject:masterfrom
privkeyio:fix/null-tx-truncated-block

Conversation

@kwsantiago

@kwsantiago kwsantiago commented Sep 8, 2026

Copy link
Copy Markdown

pull_bitcoin_tx_only() returns NULL once the cursor runs out, and the next line writes through it, so a block ending part-way through a transaction segfaults lightningd instead of being rejected by the if (!p || len) check below.

Truncating the block already in run-bitcoin_block_from_hex to 162 bytes is enough:

Program received signal SIGSEGV, Segmentation fault.
bitcoin/block.c:213	b->tx[i]->chainparams = chainparams;

The block arrives from the chain backend through getrawblockbyheight, so this needs a bitcoind serving a malformed block, not anything a peer can send.

The test walks every truncation of the existing test block, covering the header and each transaction boundary rather than one hand-picked length.

Second commit fixes a leak that test exposed: linear_tx hangs off ctx, so the failure paths free b and leave it behind. Under the Makefile's valgrind invocation, 2,143,160 bytes in 2,849 blocks before, none after.

Changelog-None

@kwsantiago
kwsantiago force-pushed the fix/null-tx-truncated-block branch from f216449 to 439071e Compare September 8, 2026 19:22
Andezion
Andezion previously approved these changes Sep 14, 2026

@Andezion Andezion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me

pull_bitcoin_tx_only() returns NULL once the cursor runs out, and the
next line writes through it, so a block that ends mid-transaction
segfaults lightningd instead of being rejected.

The block comes from the chain backend via getrawblockbyheight, so this
needs a bitcoind serving a malformed or unparseable block rather than
anything a peer can send.

Test walks every truncation of the existing test block; it segfaults at
162 bytes without the fix.
@daywalker90
daywalker90 force-pushed the fix/null-tx-truncated-block branch from 439071e to 1f722b5 Compare September 16, 2026 16:10
@daywalker90

Copy link
Copy Markdown
Collaborator

CI caught a leak:

==6223== 2,143,160 bytes in 2,849 blocks are still reachable in loss record 1 of 1
==6223==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6223==    by 0x11E2EA: allocate (tal.c:256)
==6223==    by 0x11EADF: tal_alloc_ (tal.c:473)
==6223==    by 0x11EB95: tal_alloc_arr_ (tal.c:517)
==6223==    by 0x1103BE: bitcoin_block_from_hex (block.c:158)
==6223==    by 0x110906: main (run-bitcoin_block_from_hex.c:107)
==6223== 
make: *** [Makefile:941: unittest/bitcoin/test/run-bitcoin_block_from_hex] Error 7

linear_tx hangs off ctx, so the failure paths free b and leave it behind.
@kwsantiago
kwsantiago force-pushed the fix/null-tx-truncated-block branch from 37c4c2c to 6d82425 Compare September 16, 2026 17:18
@kwsantiago
kwsantiago requested a review from Andezion September 16, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants