RPC Bug: possible null pointer dereference in getblockchaininfo() #11158

Describe the issue

In src/rpc/blockchain.cpp, in getblockchaininfo(), there is a null pointer dereference accessing block->nHeight, since block is set to nullptr in case fPruneMode is true and chainActive.Tip() returns nullptr:

    if (fPruneMode)
    {
        CBlockIndex *block = chainActive.Tip();
        while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
            block = block->pprev;

        obj.push_back(Pair("pruneheight",        block->nHeight));
    }

What version of bitcoin-core are you using?

commit 07c92b98e2a0a29d19b5ccdbbfa9addc1e80b306

Do you have steps to reproduce?

chainActive.Tip() returns nullptr

RPC will not leave warmup mode before chainActive.Tip() has a non-null value, so this can AFAIK never happen.

@laanwj I don't, static analysis only.

In that case, perhaps an exception should be thrown in the calling code in case chainActive.Tip() returns nullptr? The condition in the while loop checks for block == nullptr, hinting that this situation is possible.

The condition in the while loop checks for block == nullptr

But it isn't needed. This should be enough:

assert(block);

while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    block = block->pprev;
}

@promag I'm just getting familiar with the conventions here; is an assert considered a better alternative here? Since it would have no effect in Release builds, theoretically block could still be nullptr and its dereference lead to Undefined Behavior, i.e., possible security issues. Also, static analyzers would still be upset about the possibility of block being nullptr when being dereferenced in Release builds.

What do you suggest? if (!block) throw ...;?

@promag Yes, it makes sense to me. However, it really depends on the project's conventions. assert has the advantage of not incurring any performance penalty. Personally I think in a project like bitcoin security is more important, so if it were up to me I would choose an exception (or some form of assert which takes effect in release builds).

An assertion would be OK here - we don't allow disabling them, so they're also in release builds. Throwing an exception is overkill for something that would be a program bug. This cannot happen, after all.

BTW this seems a duplicate of #10619, please take the discussion there.