New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 21, 2017

Conversation

Projects
None yet
8 participants
@practicalswift
Member

practicalswift commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jan 14, 2017

Member

I think this can be a static_assert

Member

theuni commented Jan 14, 2017

I think this can be a static_assert

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Jan 14, 2017

Member

@theuni But static_assert(…) would require a constant expression (and hence not pit != NULL), right? :-)

Member

practicalswift commented Jan 14, 2017

@theuni But static_assert(…) would require a constant expression (and hence not pit != NULL), right? :-)

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jan 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!

Member

theuni commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Jan 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?

Contributor

JeremyRubin commented Jan 14, 2017

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 fanquake added the P2P label Jan 14, 2017

@robmcl4

This comment has been minimized.

Show comment
Hide comment
@robmcl4

robmcl4 Jan 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.

Contributor

robmcl4 commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Jan 14, 2017

Member

@JeremyRubin @robmcl4 Now changed from assert to nullcheck!

Member

practicalswift commented Jan 14, 2017

@JeremyRubin @robmcl4 Now changed from assert to nullcheck!

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Feb 27, 2017

Member

Any changes needed before merge? :-)

Member

practicalswift commented Feb 27, 2017

Any changes needed before merge? :-)

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

This comment has been minimized.

@paveljanik

paveljanik Feb 28, 2017

Contributor

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

@paveljanik

paveljanik Feb 28, 2017

Contributor

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

This comment has been minimized.

@practicalswift

practicalswift Feb 28, 2017

Member

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

@practicalswift

practicalswift Feb 28, 2017

Member

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

This comment has been minimized.

@paveljanik

paveljanik Feb 28, 2017

Contributor

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

@paveljanik

paveljanik Feb 28, 2017

Contributor

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

[net] Avoid possibility of NULL pointer dereference in MarkBlockAsInF…
…light(...)

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).
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Feb 28, 2017

Member

@paveljanik Good point. Changed to if (pit) { … } for consistency and pushed. Looks good? :-)

Member

practicalswift commented Feb 28, 2017

@paveljanik Good point. Changed to if (pit) { … } for consistency and pushed. Looks good? :-)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 14, 2017

Contributor

utACK 95543d8

Sorry for the delay.

Contributor

paveljanik commented Jun 14, 2017

utACK 95543d8

Sorry for the delay.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 19, 2017

Contributor

utACK 95543d8

Contributor

TheBlueMatt commented Jun 19, 2017

utACK 95543d8

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 20, 2017

Member

utACK 95543d8

Member

sipa commented Jun 20, 2017

utACK 95543d8

@sipa sipa merged commit 95543d8 into bitcoin:master Jun 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jun 21, 2017

Merge #9549: [net] Avoid possibility of NULL pointer dereference in M…
…arkBlockAsInFlight(...)

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

Tree-SHA512: 80fd4f2712f20377185bd8d319255f2c54ae47b54c706f7e0d384a0a6ade1465ceb6e2a4a7f7b51987a659524474a954eddf228865ebb3fc513948b5b6d7ab6d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment