[rpc]Avoid possibility of NULL pointer dereference in getblockchaininfo(...) #10619

The variable block is initialized by the return value of Tip() which may be NULL. The while loop condition takes this into account and checks for nullness before dereferencing. If Tip() returns null, the while loop is never executed and the null pointer is dereferenced right after ( block->nHeight)

With this fix, there is a single check for nullness of block. A JSONRPCError is thrown in this case. After that, block is only assigned non-null values.

The code block->nHeight is never executed if block is NULL. The while loop condition is also simplified.

Under which circumstances can Tip() be NULL? Isn't there always at least the genesis block?

If it is NULL, it crashes anyway as there are many more instances of Tip() dereferences without NULL checks in the same function alone (i.e. for bestblockhash, chainwork`, etc.).

If it can be NULL, then we should define how the rpc output should look like in this case, and fix all instances / make the output consistent.

If it can't be null, we should ensure that in the Tip() function.

If Tip() never returns NULL, then checking block for nullness in the following statment: while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) is redundant.

Do you think this should become: while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) with the rest of the code remaining as is?

Since Tip() can theoretically return NULL, I think that the best coding practice is to check for nullness somehow.

If Tip() never returns NULL [...] Do you think this should become: while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) with the rest of the code remaining as is?

In that case, yes.

As I said, if tip actually returns null, the function will crash as it is today, so we should either fix all of that (and document/test it), or have tip() not return null (and work backwards to ensure correctness).

I think that checking the return value of a function that returns a pointer for nullness is a good practice that will safeguard the code from dereferencing a null pointer either under the current or under any other future implementation of Tip().

Actually, I see that in other parts of the code the return value of Tip() is checked for nullness. Do you think this PR should be merged to ensure consistency with that, before a more generic solution is investigated?

C++ has the problem that pointers are used for two orthogonal reasons: 1) to avoid unneeded copies, and 2) to encode the possibility of NULL. If a function can't return NULL but still returns a pointer (for the first reason), I don't think callers need to check, as it complicates the code for no good reason. If someone changes the function to return null in the future, it's an API change (unfortunately not caught by the compiler) and they need to fix the callsites.

There are probably edge cases in which Tip() returns NULL (on startup and shutdown, and maybe one can even mark the genesis block as invalid via an rpc command?).

What about throwing an JSONRPCError in the beginning of the function? If Tip is null and the chain is empty, the whole call doesn't make a lot of sense.

I have modified the commit for this PR. Now an JSONRPCError is thrown before any return value of Tip() is used.

Thank you.

utACK cf67a3c64d0813e1131bc30e7f628b7bfd21fc96 for the code.

As for the concept, I'd rather have more people confirm if this makes sense.

Concept ACK.

In addition to the check you added I suggest adding an assertion before accessing block->nHeight:

         CBlockIndex *block = chainActive.Tip();
         while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
             block = block->pprev;
+        assert(block && "An empty blockchain should not be possible here.");
         obj.push_back(Pair("pruneheight",        block->nHeight));

I'm assuming a state transition from chainActive.Tip() != nullptr (blockchain non-empty) to chainActive.Tip() == nullptr (blockchain empty) is impossible in the current code? If so I think it makes sense to make that clear (via an assertion) since the code here appears to be working under that assumption. It will have the added benefit of silencing this NULL pointer dereference warning for static analysis tools that don't pick up this subtle state change limitation (if such a limitation exists).

Thanks for reporting this @pavlosantoniou. You beat me to reporting it! :-) I had this in my backlog of possible NULL pointer dereferences to report, but as I try to limit the number of open PR:s pertaining to the same class of issues I figured I'd await the merge of a similar issue I reported in #9549:

[net] Avoid possibility of NULL pointer dereference in MarkBlockAsInFlight(...)

Thank you all for your comments, I have added the assertion in the commit for this PR.

@pavlosantoniou Oh, I forgot to mention: I think you should restore the null check in the while loop too:

-        while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
+        while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))

@practicalswift that shouldn't be necessary with the check before that the tip is not null?

If you are afraid that the return value changes between two calls of Tip() in that function, the result should just be stored in a variable in the beginning and reused.

@pavlosantoniou I suggest using the following patch (while removing the other changes):

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 8f7f768..baf8dae 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1165,6 +1165,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
             + HelpExampleRpc("getblockchaininfo", "")
         );

+    assert(chainActive.Tip() && "An empty blockchain should not be possible here.");
     LOCK(cs_main);

     UniValue obj(UniValue::VOBJ);
@@ -1196,6 +1197,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
         while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
             block = block->pprev;

+        assert(block && "An empty blockchain should not be possible here.");
         obj.push_back(Pair("pruneheight",        block->nHeight));
     }
     return obj;

@practicalswift why are the asserts needed? I guess they don't hurt, on the other hand, those asserts check something trivial. @pavlosantoniou can you assign Tip() to one var and reuse it in the rest of the function? Then there should be no doubt about its value.

@benma The asserts serve as documentation of an assumption that is not obvious from reading the source code. As an added bonus it guides static analysis tools such as clang-tidy which will otherwise warn about a potential NULL pointer dereference here.

@practicalswift thanks, I didn't know about this tool. How many warnings like this are there?

@practicalswift Concerning your proposed patch, I agree with the addition of the two assertions. But since block is checked for nullness in: while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) the possibility of nullness is implied. Shouldn't block be dereferenced conditionally then ? e.g. in: obj.push_back(Pair("pruneheight", block->nHeight));

How about @benma 's proposal, to save the return value of Tip() once in the beginning, maybe have an assertion there, and then use the saved value throughout the function with no additional checks.

@pavlosantoniou

In this code …

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

… then nullptr check does only cover the while. After the while the block variable could still be set to nullptr – that's why the assertion is needed :-)

I have updated the commit with the suggested changes.

Yes @benma you are right! Corrected

ACK be42c0d80e1de2bfd4f2658014eda18cab5a398d

utACK be42c0d

@benma There are currently three null pointer dereference warnings reported by clang-tidy's module clang-analyzer-core.NullDereference:

• Addressed in PR #9549 – net_processing.cpp:345:14: warning: Dereference of null pointer (loaded from variable 'pit')
• Addressed in PR #10619 – rpc/blockchain.cpp:1199:50: warning: Access to field 'nHeight' results in a dereference of a null pointer (loaded from variable 'block')
• Addressed in PR #10638 – rpc/mining.cpp:560:20: warning: Access to field 'nNonce' results in a dereference of a null pointer (loaded from variable 'pblock')

There are a ton of places we assume chainActive.Tip() is non-NULL, and init makes sure there is something there. Adding assert()s everywhere we dereference chainActive.Tip() during normal runtime seems like an excersize in too many assert()s.

@TheBlueMatt I think it makes sense to add an assertion in this case to guide clang-tidy. This is one of only two remaining null pointer dereference warnings emitted by clang-tidy's module clang-analyzer-core.NullDereference.

@sipa @benma Is there something more I can do for this PR?

utACK be42c0d80e1de2bfd4f2658014eda18cab5a398d (edit: just noticed I already ACK'd this commit before) @pavlosantoniou I feel like #10619 (comment) hasn't been addressed, really (i.e. why block && should be restored). Not a blocker of course.

@benma Are you refering to the saving-and-reusing-block's-value part of the comment?

Yes, with regards to the block && part that you originally removed. In other words, I am not sure why @practicalswift requested this: #10619 (comment)

utACK be42c0d80e1de2bfd4f2658014eda18cab5a398d

Agree with @TheBlueMatt.

"Avoid possibility of NULL pointer dereference" is overly alarmist as this cannot happen in practice.

I agree with @TheBlueMatt . We make sure that chainActive.Tip() is non-zero before allowing RPC calls, so asserting everywhere before use should be unnecessary. And adding it only on one place is even less useful, as it's not consistent.

If it fixes an automated warning, I suppose I'm fine with this, though, really, the number of automated warning fixes is getting a bit tiring.

Which automated warning, from what?

@laanwj I was referring to the one at #10619 (comment)

Just a bit of advice, I'd prefer that titles like "Avoid null deference" be reserved for cases where we believe one is actually possible. Among other reasons, commits like this in the history will suppress legitimate issue reports when someone sees a crash then "oh, I guess thats been fixed". :)

In terms of guarding them with asserts, sure. That is a reasonable thing to do which we do elsewhere. But could PRs like this please get titles like "Add a null guard in function X" unless we strongly suspect there was a real issue? (and obviously, if it was reachable with a null here, an assert wouldn't be the right fix.)

@sipa Agree about the wording! @laanwj I think the reason for the static analyzer warning in this case (and other cases reported as "possible null pointer dereference") is that a NULL check is performed earlier in the code which would imply that the variable can be NULL (not necessarily in practice, but in the reasoning of the analyzer). A dereference takes place later in the code where no NULL check exists and the static analyzer then concludes that there is a possibility of a NULL pointer dereference.

More specifically in this case:

    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));
    }

In this case:

1. while (block && …) implies that block can theoretically be NULL.
2. block->nHeight - dereference without checking for NULL. Warning!

Both clang-tidy and cppcheck follow that line of reasoning:

# clang-tidy
rpc/blockchain.cpp:1199:50: warning: Access to field 'nHeight' results in a dereference of a null pointer (loaded from variable 'block') [clang-analyzer-core.NullDereference]

# cppcheck
[src/rpc/blockchain.cpp:1199] -> [src/rpc/blockchain.cpp:1196]: (warning) Either the condition 'block' is redundant or there is possible null pointer dereference: block.

So if we're sure that chainActive.Tip() cannot be NULL in this context the warning can be avoided by removing the NULL-check in the while-loop, or adding a assert before the dereference.

(In addition to adding a a NULL-check before the dereference of course :-))

Perhaps removing the NULL-check in the while-loop and maybe adding an assert before the while loop is the best way to go if we are sure that chainActive.Tip() cannot be NULL in this context. That removes the redundant check and documents the assumption made when doing said removal.

This will silence the clang-tidy and cppcheck warnings:

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index d65e107..1915a45 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1193,7 +1193,8 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
     if (fPruneMode)
     {
         CBlockIndex *block = chainActive.Tip();
-        while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
+        assert(block && "chainActive.Tip() != nullptr assumed");
+        while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))
             block = block->pprev;

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

Note that the assert(…) is technically not needed to get rid of the analyzer warnings.

Closing for now, discussing has died out, and this needs a rebase.