rpc: fix getrawtransaction segfault #29003

mzumsande · 2023-12-05T18:29:19Z

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.

DrahtBot · 2023-12-05T18:29:22Z

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.

maflcko · 2023-12-05T18:48:47Z

lgtm ACK 494a926

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

maflcko · 2023-12-05T18:52:44Z

I guess this was introduced in f866971

maflcko · 2023-12-05T18:55:14Z

src/rpc/rawtransaction.cpp

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


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.

maflcko · 2023-12-05T18:56:56Z

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

tACK 494a926

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

mzumsande · 2023-12-05T19:09:19Z

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

Will add one soon.

pablomartin4btc · 2023-12-05T19:53:36Z

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(?):

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

Or even return false; earlier before the assert.

mzumsande · 2023-12-05T20:10:04Z

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 · 2023-12-05T21:14:39Z

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

jonatack · 2023-12-05T21:29:01Z

Concept ACK

mzumsande · 2023-12-05T21:43:30Z

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.

This fails on master without the previous commit.

theStack · 2023-12-05T23:10:07Z

Concept ACK

theStack

Tested ACK 9075a44

Reproduced the crash on master with my pruned node manually via

$ ./src/bitcoin-cli getrawmempool | head -n 2
[
    "sometxid...",
$ ./src/bitcoin-cli getrawtransaction sometxid... 2
error: timeout on transient error: Could not connect to the server 127.0.0.1:8332 (error code 1 - "EOF reached")

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

-----
bitcoind 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.