Merge bitcoin/bitcoin#29003: rpc: fix getrawtransaction segfault

9075a44646 test: add regression test for the getrawtransaction segfault (Martin Zumsande)
494a926d05 rpc: fix getrawtransaction segfault (Martin Zumsande)

Pull request description:

  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

ACKs for top commit:
  maflcko:
    lgtm test-was-added ACK 9075a44646
  theStack:
    Tested ACK 9075a44646

Tree-SHA512: 0f7ed52579487196c206e16b45582b64e4b02ecf2a2eb0a31d2f3b52415bc9c64278cb94259314ef14ab7fb393c6195f79b3027d6de471d67614e51474498b11
This commit is contained in:
fanquake 2023-12-06 14:28:54 +00:00
commit dde7ac5c70
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
2 changed files with 13 additions and 5 deletions

View file

@ -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());
return result;
}
@ -405,8 +405,7 @@ static RPCHelpMan getrawtransaction()
CBlock block;
const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))};
if (tx->IsCoinBase() ||
!blockindex || is_block_pruned ||
if (tx->IsCoinBase() || is_block_pruned ||
!(chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex) && chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))) {
TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
return result;

View file

@ -32,6 +32,7 @@ from test_framework.script import (
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
)
from test_framework.wallet import (
@ -70,7 +71,7 @@ class RawTransactionsTest(BitcoinTestFramework):
self.extra_args = [
["-txindex"],
["-txindex"],
[],
["-fastprune", "-prune=1"],
]
# whitelist all peers to speed up tx relay / mempool sync
for args in self.extra_args:
@ -85,7 +86,6 @@ class RawTransactionsTest(BitcoinTestFramework):
self.wallet = MiniWallet(self.nodes[0])
self.getrawtransaction_tests()
self.getrawtransaction_verbosity_tests()
self.createrawtransaction_tests()
self.sendrawtransaction_tests()
self.sendrawtransaction_testmempoolaccept_tests()
@ -94,6 +94,8 @@ class RawTransactionsTest(BitcoinTestFramework):
if self.is_specified_wallet_compiled() and not self.options.descriptors:
self.import_deterministic_coinbase_privkeys()
self.raw_multisig_transaction_legacy_tests()
self.getrawtransaction_verbosity_tests()
def getrawtransaction_tests(self):
tx = self.wallet.send_self_transfer(from_node=self.nodes[0])
@ -243,6 +245,13 @@ class RawTransactionsTest(BitcoinTestFramework):
coin_base = self.nodes[1].getblock(block1)['tx'][0]
gottx = self.nodes[1].getrawtransaction(txid=coin_base, verbosity=2, blockhash=block1)
assert 'fee' not in gottx
# check that verbosity 2 for a mempool tx will fallback to verbosity 1
# Do this with a pruned chain, as a regression test for https://github.com/bitcoin/bitcoin/pull/29003
self.generate(self.nodes[2], 400)
assert_greater_than(self.nodes[2].pruneblockchain(250), 0)
mempool_tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid']
gottx = self.nodes[2].getrawtransaction(txid=mempool_tx, verbosity=2)
assert 'fee' not in gottx
def createrawtransaction_tests(self):
self.log.info("Test createrawtransaction")