Potential nullptr dereference in getblockchaininfo #6393

issue GSPP opened this issue on July 7, 2015

GSPP commented at 8:59 PM on July 7, 2015: none
getblockchaininfo has the following code:
```
        CBlockIndex *block = chainActive.Tip();
        while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
            block = block->pprev;

        obj.push_back(Pair("pruneheight",        block->nHeight));
```
This code assumes that block can be null. After the loop block can still be null if block was initially null or if block->pprev was null.

Then, block->nHeight leads to a null pointer dereference which is UB.
jonasschnelli commented at 9:14 PM on July 7, 2015: contributor

IMO block->prev can't be NULL because we do && block->prev in the while condition.

I'm not sure if a chainActive.Tip() can ever be NULL but I also would say it requires a if(block) before accessing nHeight.
GSPP commented at 9:24 PM on July 7, 2015: none

True.
sipa commented at 11:08 PM on July 9, 2015: member

chainActive.Tip() can only be NULL when you're reindexing, and haven't processed the on-disk genesis block.
GSPP commented at 8:53 AM on July 10, 2015: none

OK, I understand. Maybe this should be a runtime assertion. Even the slightest potential for Undefined Behavior in a Bitcoin software is reason for concern.
laanwj commented at 8:00 AM on July 29, 2015: member

It's set up such that the control flow will never leave AppInit2 without a genesis block set. So it is in practice impossible to hit this. Feel free to add an assertion if that makes you feel better, though.
laanwj closed this on Feb 9, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#7493 rpc: add null assert