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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:avoid-potential-null-pointer-dereference-in-markblockasinflight changing 1 files +3 −1

practicalswift commented at 1:09 AM on January 14, 2017: contributor
In the case that the branch ...
```
if (itInFlight != mapBlocksInFlight.end() && itInFlight->second.first == nodeid) {
```
... is taken, there was prior to this commit an implicit assumption that MarkBlockAsInFlight(...) was being called with its fifth and optional argument (pit) being present (and non-NULL).

There are three calls to MarkBlockAsReceived(...) and for two of these the optional fifth argument is not supplied, which means that pit is being set to the default value of NULL.

This commit adds an assertion which makes the mentioned assumption explicit, and removes the possibility of a NULL pointer dereference.
theuni commented at 1:12 AM on January 14, 2017: member

I think this can be a static_assert
practicalswift commented at 1:39 AM on January 14, 2017: contributor

@theuni But static_assert(…) would require a constant expression (and hence not pit != NULL), right? :-)
theuni commented at 2:00 AM on January 14, 2017: member

Hmm, right. I suppose this would have to be a templated arg overloaded with nullptr_t to work, which would obviously be silly here. Nevermind!
JeremyRubin commented at 2:33 AM on January 14, 2017: contributor

The right change here might be to just only write to the pointer if it is given as argument... (e.g., a nullcheck branch not an assert)

Will, in normal execution, the assert ever panic?
fanquake added the label P2P on Jan 14, 2017
robmcl4 commented at 5:57 AM on January 14, 2017: contributor

From how I read it the assertion will never panic, but I agree this should be a nullcheck branch.

I see the two calls to MarkBlockAsReceived(..) which exclude the final param, on lines 2260 and 3192. Both are protected by cs_main. The former will only retrieve blocks not marked in flight and the latter uses FindNextBlocksToDownload(..), which itself only provides blocks not marked as in flight. So the assertion's branch will never be exercised in these two cases.
practicalswift force-pushed on Jan 14, 2017
practicalswift commented at 9:01 AM on January 14, 2017: contributor

@JeremyRubin @robmcl4 Now changed from assert to nullcheck!
practicalswift cross-referenced this on Jan 18, 2017 from issue 6696b46: Clang Static Analyzer Report by kallewoof
practicalswift commented at 10:19 PM on February 27, 2017: contributor

Any changes needed before merge? :-)

in src/net_processing.cpp:None in a1d29869e5 outdated

 337 | @@ -338,7 +338,9 @@ bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const Consensus::Pa
 338 |      // Short-circuit most stuff in case its from the same node
 339 |      map<uint256, pair<NodeId, list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash);
 340 |      if (itInFlight != mapBlocksInFlight.end() && itInFlight->second.first == nodeid) {
 341 | -        *pit = &itInFlight->second.second;
 342 | +        if (pit != NULL) {

paveljanik commented at 12:33 PM on February 28, 2017:

Why do you use different style then the style used just before return true?

practicalswift commented at 12:36 PM on February 28, 2017:

@paveljanik Are you referring to the braces? if (…) … vs. if (…) { … }?

paveljanik commented at 2:13 PM on February 28, 2017:

if (pit) vs if (pit != NULL)

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

In the case that the branch ...

    if (itInFlight != mapBlocksInFlight.end() && itInFlight->second.first == nodeid) {

... is taken, there was prior to this commit an implicit assumption that
MarkBlockAsInFlight(...) was being called with its fifth and optional
argument (pit) being present (and non-NULL).

95543d8747

practicalswift force-pushed on Feb 28, 2017
practicalswift commented at 2:52 PM on February 28, 2017: contributor

@paveljanik Good point. Changed to if (pit) { … } for consistency and pushed. Looks good? :-)
practicalswift cross-referenced this on May 10, 2017 from issue Remove unused argument from MarkBlockAsInFlight(...) by practicalswift
paveljanik commented at 7:47 AM on June 14, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/9549/commits/95543d8747cbf7c1945ac36c36031ae40152cf2f

Sorry for the delay.
practicalswift cross-referenced this on Jun 17, 2017 from issue [rpc]Avoid possibility of NULL pointer dereference in getblockchaininfo(...) by pavlosantoniou
TheBlueMatt commented at 6:41 PM on June 19, 2017: contributor

utACK 95543d8747cbf7c1945ac36c36031ae40152cf2f
sipa commented at 11:19 PM on June 20, 2017: member

utACK 95543d8747cbf7c1945ac36c36031ae40152cf2f
sipa merged this on Jun 21, 2017
sipa closed this on Jun 21, 2017
sipa referenced this in commit b33ca14f59 on Jun 21, 2017
MiWCryptoCurrency cross-referenced this on Dec 9, 2017 from issue Expose csv in transactionview (CSV export support) by MiWCryptoCurrency
PastaPastaPasta referenced this in commit ea47c6393e on Jul 5, 2019
PastaPastaPasta referenced this in commit 7f2daf6dab on Jul 5, 2019
PastaPastaPasta referenced this in commit 941c0323e6 on Jul 6, 2019
PastaPastaPasta referenced this in commit 269a7eae55 on Jul 8, 2019
PastaPastaPasta referenced this in commit 9205b1986a on Jul 9, 2019
PastaPastaPasta referenced this in commit a8fcc80a16 on Jul 9, 2019
barrystyle referenced this in commit 90cd30066a on Jan 22, 2020
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#9573 6696b46: Clang Static Analyzer Report #10319 Remove unused argument from MarkBlockAsInFlight(...)#10619 [rpc]Avoid possibility of NULL pointer dereference in getblockchaininfo(...)