Skip to content
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

validation: assert that pindexPrev is non-null when required #14834

Conversation

Projects
None yet
9 participants
@kallewoof
Copy link
Member

commented Nov 29, 2018

In ContextualCheckBlock, we are checking if pindexPrev == nullptr conditionally at the start, but then assume it is non-null later. This removes the latter assumption.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Is it even hypothetically possible for LOCKTIME_MEDIAN_TIME_PAST to be set on a genesis block?

Precedence is non-obvious in this case, so prefer adding parenthesis for clarity at least.

@fanquake fanquake added the Validation label Nov 29, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

That's a good point. Is that the only case ContextualCheckBlock is called with pindexPrev == nullptr? (When block is the genesis block)

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

     const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;
@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Yeah, that pretty much proves it. It could change in the future, though, so I think the change is still warranted. It also plugs a static analyzer warning about null dereferencing, so there is that.

@kallewoof kallewoof force-pushed the kallewoof:20181129-contextualcheckblock-pindexprev-nullness branch Nov 29, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

If it changes in the future, your modification here is a security vulnerability, since the block must be checked with the correct min locktime.

@kallewoof kallewoof force-pushed the kallewoof:20181129-contextualcheckblock-pindexprev-nullness branch Nov 29, 2018

@kallewoof kallewoof changed the title validation: do not break assumption that pindexPrev may be null validation: assert that pindexPrev is non-null when required Nov 29, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

@luke-jr I changed the approach and am now asserting that pindexPrev is non-null for the case where the lock time flags have LOCKTIME_MEDIAN_TIME_PAST. That seems more aligned with your reasoning, I think, and I agree makes more sense in this case.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

I guess that looks okay, although I found the logic relatively hard to follow.

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Actually, I can just move it into the if block above. Doing.

@kallewoof kallewoof force-pushed the kallewoof:20181129-contextualcheckblock-pindexprev-nullness branch to fbaaf78 Nov 29, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK fbaaf78

Very nice to get rid of this warning and thereby increase the signal-to-noise in static analyzer output! Explicit assumptions are better than implicit assumptions. Thanks!

@promag
Copy link
Member

left a comment

utACK fbaaf78.

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

@promag Maybe I'm not looking closely enough -- how does that improve the patch?

@promag

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

@kallewoof yeah, it doesn't improve this patch, I've updated my comment shortly after, but now I've removed the suggestion.

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Gotcha!

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK fbaaf78

2 similar comments
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK fbaaf78

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

utACK fbaaf78

@laanwj laanwj merged commit fbaaf78 into bitcoin:master Dec 13, 2018

2 checks passed

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

laanwj added a commit that referenced this pull request Dec 13, 2018

Merge #14834: validation: assert that pindexPrev is non-null when req…
…uired

fbaaf78 validation: assert that pindexPrev is non-null when required (Karl-Johan Alm)

Pull request description:

  In `ContextualCheckBlock`, we are checking if `pindexPrev == nullptr` conditionally at the start, but then assume it is non-`null` later. This removes the latter assumption.

Tree-SHA512: 95f1e9dc839b2cc0e099d155e6180634ece8c6760d00b53e7d27128762e64c92e82d98a5f4a5786b48a4851b17cdbb4b667d3b6a99adb651256e2032de67d05c

@kallewoof kallewoof deleted the kallewoof:20181129-contextualcheckblock-pindexprev-nullness branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.