rpc: fix getrawtransaction segfault

mzumsande commented at 6:29 pm on December 5, 2023: contributor

The crash, reported in #28986, happens when calling getrawtransaction for any mempool transaction with verbosity=2, while pruning, because the rpc calls IsBlockPruned(const CBlockIndex* pblockindex), which dereferences pblockindex without a check.

For ease of backporting this PR fixes it just locally in rpc/rawtransaction.cpp by moving the check for!blockindex up so that IsBlockPruned() will not be called with a nullptr. We might also want to change IsBlockPruned() so it doesn’t crash when called with a nullptr, but I didn’t do that here.

Fixes #28986

The crash would happen when querying a mempool transaction with verbosity=2, while pruning.

494a926d05

DrahtBot commented at 6:29 pm on December 5, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, maflcko
Concept ACK	jonatack
Stale ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Dec 5, 2023

maflcko added the label Needs backport (25.x) on Dec 5, 2023

maflcko added the label Needs backport (26.x) on Dec 5, 2023

maflcko added this to the milestone 25.2 on Dec 5, 2023

maflcko removed this from the milestone 25.2 on Dec 5, 2023

maflcko added this to the milestone 27.0 on Dec 5, 2023

maflcko commented at 6:48 pm on December 5, 2023: member

lgtm ACK 494a926d05df44b60b3bc1145ad2a64acf96f61b

A test wouldn’t hurt, I’d say.

fanquake removed this from the milestone 27.0 on Dec 5, 2023

fanquake added this to the milestone 26.1 on Dec 5, 2023

maflcko commented at 6:52 pm on December 5, 2023: member

I guess this was introduced in https://github.com/bitcoin/bitcoin/commit/f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9

in src/rpc/rawtransaction.cpp:400 in 494a926d05 outdated

395@@ -396,7 +396,7 @@ static RPCHelpMan getrawtransaction()
396         LOCK(cs_main);
397         blockindex = chainman.m_blockman.LookupBlockIndex(hash_block);
398     }
399-    if (verbosity == 1) {
400+    if (verbosity == 1 || !blockindex) {
401         TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());

maflcko commented at 6:55 pm on December 5, 2023:

unrelated: I don’t understand the silent fallback to verbose=1, when verbose=2 is not available. Either this should be documented, or an error should be thrown (same below).

Though, this is unrelated.

bitcoin deleted a comment on Dec 5, 2023

maflcko commented at 6:56 pm on December 5, 2023: member

We might also want to change IsBlockPruned() so it doesn’t crash when called with a nullptr, but I didn’t do that here.

I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.

pablomartin4btc approved

pablomartin4btc commented at 7:08 pm on December 5, 2023: member

tACK 494a926d05df44b60b3bc1145ad2a64acf96f61b

I’ve managed to reproduce the issue, this PR fixes it.

mzumsande commented at 7:09 pm on December 5, 2023: contributor

A test wouldn’t hurt, I’d say.

Will add one soon.

pablomartin4btc commented at 7:53 pm on December 5, 2023: member

We might also want to change IsBlockPruned() so it doesn’t crash when called with a nullptr, but I didn’t do that here.

I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.

Sorry, why not fixing the issue directly here(?):

0--- a/src/node/blockstorage.cpp
1+++ b/src/node/blockstorage.cpp
2@@ -585,7 +585,7 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
3 bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
4 {
5     AssertLockHeld(::cs_main);
6-    return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
7+    return (m_have_pruned && (pblockindex != nullptr) && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
8 }

Or even return false; earlier before the assert.

mzumsande commented at 8:10 pm on December 5, 2023: contributor

Sorry, why not fixing the issue directly here(?):

I initially decided against this because

calling IsBlockPruned() doesn’t make any sense in the first place when there is no block.
There were lots of refactors in the blockstorage module over the last year and I wasn’t sure how cleanly this would backport.

I think that it’s not one or the other, both changes make sense. However, to avoid the question if IsBlockPruned() should return true or false for a nullptr, it would probably be better change it to use a reference as maflcko suggested.

achow101 commented at 9:14 pm on December 5, 2023: member

Perhaps a test for this case? Edit: Oh, one has already been requested.

jonatack commented at 9:29 pm on December 5, 2023: member

Concept ACK

mzumsande commented at 9:43 pm on December 5, 2023: contributor

I added a regression test (that will segfault on master). In order to trigger this, it’s not enough to enable pruning, you have to actually have pruned a block, so I did this with -fastprune in the test. Because this will probably seem a bit random to someone reading the test without this context, I added a comment linking to this PR. [Edit] Also had to reorder the test cases because the legacy-wallet tests don’t work with a pruned chain.

test: add regression test for the getrawtransaction segfault

This fails on master without the previous commit.

9075a44646

mzumsande force-pushed on Dec 5, 2023

DrahtBot added the label CI failed on Dec 5, 2023

theStack commented at 11:10 pm on December 5, 2023: contributor

Concept ACK

DrahtBot removed the label CI failed on Dec 6, 2023

theStack approved

theStack commented at 1:58 am on December 6, 2023: contributor

Tested ACK 9075a446461ccbc446d21af778aac50b604f39b3

Reproduced the crash on master with my pruned node manually via

0$ ./src/bitcoin-cli getrawmempool | head -n 2
1[
2    "sometxid...",
3$ ./src/bitcoin-cli getrawtransaction sometxid... 2
4error: timeout on transient error: Could not connect to the server 127.0.0.1:8332 (error code 1 - "EOF reached")
5
6Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
7
8-----
9bitcoind crashes with "Segmentation fault (core dumped)"

and verified that the PR fixes it. Also checked that the test crashes if cherry-picked on master, and succeeds in the PR branch.

DrahtBot requested review from maflcko on Dec 6, 2023

DrahtBot requested review from pablomartin4btc on Dec 6, 2023

DrahtBot requested review from jonatack on Dec 6, 2023

bitcoin deleted a comment on Dec 6, 2023

maflcko commented at 8:22 am on December 6, 2023: member

lgtm test-was-added ACK 9075a446461ccbc446d21af778aac50b604f39b3

DrahtBot removed review request from maflcko on Dec 6, 2023

fanquake merged this on Dec 6, 2023

fanquake closed this on Dec 6, 2023

fanquake removed the label Needs backport (26.x) on Dec 6, 2023

fanquake referenced this in commit ccc96676cb on Dec 6, 2023

fanquake referenced this in commit 94b6d6349f on Dec 6, 2023

fanquake commented at 2:48 pm on December 6, 2023: member

Added to #29011 for backporting to 26.x.

pablomartin4btc commented at 2:50 pm on December 6, 2023: member

ACK 9075a446461ccbc446d21af778aac50b604f39b3 on new reg test added

maflcko removed this from the milestone 26.1 on Dec 6, 2023

maflcko added this to the milestone 25.2 on Dec 6, 2023

fanquake referenced this in commit fd532a2a0e on Dec 6, 2023

fanquake referenced this in commit b2a78a0eb3 on Dec 6, 2023

fanquake commented at 2:58 pm on December 6, 2023: member

Added to #28768 for backporting to 25.x.

fanquake removed the label Needs backport (25.x) on Dec 6, 2023

fanquake referenced this in commit 557eb5f571 on Dec 6, 2023

fanquake referenced this in commit a8538ad192 on Dec 6, 2023

mzumsande deleted the branch on Dec 7, 2023

fanquake referenced this in commit 3e6a3b6f24 on Dec 8, 2023

fanquake referenced this in commit f17650dedd on Dec 8, 2023

fanquake referenced this in commit 4de83744d4 on Dec 11, 2023

fanquake referenced this in commit 59c97644bf on Dec 11, 2023

fanquake referenced this in commit 622e79e0fb on Dec 12, 2023

fanquake referenced this in commit 5097bb3389 on Jan 4, 2024

fanquake referenced this in commit b15e2e2cec on Jan 4, 2024

glozow referenced this in commit 04edf9f586 on Jan 9, 2024

fanquake referenced this in commit 041228d293 on Jan 15, 2024

fanquake referenced this in commit 31e1e035be on Jan 15, 2024

fanquake referenced this in commit 8087626cbd on Jan 16, 2024

bitcoin locked this on Dec 6, 2024

rpc: fix getrawtransaction segfault #29003

Code Coverage

Reviews