[net] Avoid possibility of NULL pointer dereference in ProcessMessage(...) #9559

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:avoid-null-pointer-deref-in-processmessage changing 1 files +2 −0

practicalswift commented at 10:23 PM on January 14, 2017: contributor
Prior to this commit there was an implicit assumption that the CBlockIndex pindexWalk (+ pindexLast and pindexWalk->pprev) being non-NULL. (If that was guaranteed pindexWalk would not be needed in while (pindexWalk && ...) on line 2233).

pindexWalk is being used in the check ...
```
if (!chainActive.Contains(pindexWalk)) {
```
CChain.Contains(...) is defined as:
```
bool Contains(const CBlockIndex *pindex) const {
    return (*this)[pindex->nHeight] == pindex;
}
```
Hence, a NULL pointer dereference in the case of a non-NULL argument to Contains(...).

This commit adds two assertion which make the mentioned assumptions explicit, and removes the possibility of a NULL pointer dereference.

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

Prior to this commit there was an implicit assumption that the
CBlockIndex pindexWalk (+ pindexLast and pindexWalk->pprev) being
non-NULL. (If that was guaranteed pindexWalk would not be needed in
"while (pindexWalk && ...)" on line 2233).

pindexWalk is being used in the check ...

    if (!chainActive.Contains(pindexWalk)) {

CChain.Contains(...) is defined as:

    bool Contains(const CBlockIndex *pindex) const {
        return (*this)[pindex->nHeight] == pindex;
    }

Hence, a NULL pointer dereference in the case of a non-NULL argument
to Contains(...).

This commit adds two assertion which make the mentioned assumptions
explicit, and removes the possibility of a NULL pointer dereference.

17536711fc

practicalswift force-pushed on Jan 14, 2017
practicalswift commented at 2:47 AM on January 15, 2017: contributor
Hrmm, when thinking of it it might be that pindexWalk is guaranteed to be non-NULL here and that the inclusion of pindexWalk in the while-loop fooled me in to thinking that NULL was an option.

If so ...
```
while (pindexWalk && !chainActive.Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
```
... could be reduced to the equivalent ...
```
while (!chainActive.Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
```
... if I'm reading this correctly?
fanquake added the label P2P on Jan 15, 2017

in src/net_processing.cpp:None in 17536711fc

2227 | @@ -2228,6 +2228,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
2228 |          // much work as our tip, download as much as possible.
2229 |          if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && chainActive.Tip()->nChainWork <= pindexLast->nChainWork) {
2230 |              vector<const CBlockIndex *> vToFetch;
2231 | +            assert(pindexLast);

paveljanik commented at 5:39 PM on January 15, 2017:

Isn't pindexLast dereferenced two lines above this? And even asserted ~ten lines above?

practicalswift commented at 6:19 PM on January 15, 2017:

Yes, you're absolutely right - the redundant check pindexWalk-check fooled me as mentioned in this comment :-) Sorry for the noise. Closing this PR!

practicalswift closed this on Jan 15, 2017
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

practicalswift

paveljanik

Labels

P2P