Fix mem access violation merkleblock #9980

Christewart · 2017-03-12T16:56:53Z

Fixing a possible memory access violation in CPartialMerkleTree::CalcHash().

This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them.

This was found with #8469, specifically using this generator which will cause a memory access violation on this test case.

dcousens · 2017-03-13T23:22:43Z

If it shouldn't happen, maybe an assert is better?

pstratem · 2017-03-14T00:22:13Z

@TheBlueMatt what was the behavior before if the elements list was empty?

pstratem · 2017-03-14T00:24:17Z

wait no that's @sipa

pstratem · 2017-03-14T00:26:32Z

nvm it returned 0 so this maintains the original behavior

TheBlueMatt · 2017-03-17T23:32:27Z

Probably best to assert that vTxid has non-zero length, as that would represent generating a tree for an invalid block.

Christewart · 2017-03-26T14:09:44Z

Changed to what @dcousens and @TheBlueMatt suggested. I think that makes sense at the end of the day. Also, from what I can tell, that assert cannot be hit from the p2p code.

NicolasDorier · 2017-03-27T06:20:32Z

I would have set fBad, so we don't risk later to reach a network reachable assert(0) by inadvertance. :)

jnewbery · 2017-04-06T15:33:09Z

Looks good. Lightly tested ACK 51802ccc4561bd5a94499c665a333517f5eadd10

if you're adding an assert, can you put some commenting in explaining the assumptions that you're testing (either around the assert or the function declaration).

Also please remove the merge commits from this PR.

EDIT: cool that this was found using your property-based testing. I've been meaning to review #8469 for a while.

Christewart · 2017-04-07T02:13:53Z

@jnewbery Addressed the concerns you had.

If you want to chat about 8469 don't be afraid to ping me on irc. I think it will allow us to streamline testing AND exhaustively test invariants better than we are now.

NicolasDorier · 2017-04-07T08:44:02Z

why no just setting fBad and returning ? I really don't like having an assert, this code might be reached from the network one day for some reason (maybe unsuspecting new feature). Putting an assert here is a tragedy waiting to happen.

jnewbery · 2017-04-07T13:36:22Z

@NicolasDorier - I had the same concerns as you, so I spent some time reading around the code to convince myself that this was ok. This code is only called from the CMerkleBlock constructor, which is only called from the gettxoutproof RPC or in response to a MSG_FILTERED_BLOCK getdata request. In both cases, we're constructing the Merkle Block from a block that's already in our mapBlockIndex. Therefore to hit this assert, we'd need there to be an invalid block in the mapBlockIndex, which would mean:

our internal data is corrupted; and
we've probably fallen pretty badly out of consensus

in which case, asserting is appropriate.

As for concerns about future code calling this without knowing about the assert - they'd have to be constructing a merkle block for a block that they hadn't already validated, which is a very bad idea! Just to make sure, Chris has now also added a comment above the assert to clarify assumptions.

Hope that allays your concerns!

TheBlueMatt · 2017-07-11T23:18:10Z

utACK bbdd771e4c1a9ae405a266c0b158bf6825acef15, though it would be nice to add a comment to CMerkleBlock noting that the block containing transactions will be asserted-on.

Christewart · 2017-07-12T15:14:14Z

@TheBlueMatt Added the comment, is this ready for merge?

EDIT: Also rebased

TheBlueMatt · 2017-07-12T15:34:09Z

src/merkleblock.h

@@ -121,6 +121,8 @@ class CPartialMerkleTree
 /**
 * Used to relay blocks as header + vector<merkle branch>
 * to filtered nodes.
+ *
+ * NOTE: This is asserted on when building CPartialMerkleTree 


Wait, what is asserted? Also note extra space at EOL here.

…kleTree::CalcHash() Adding comment to assert in PartialMerkleTree::CalcHash() Adding comment on CMerkleBlock indicating it calls something that contains an assert Removing EOL whitespace

gmaxwell

utACK.

TheBlueMatt · 2017-07-17T17:54:36Z

utACK 8276e70

sipa · 2017-07-17T18:07:34Z

utACK 8276e70

8276e70 Adding assert to avoid a memory access violation inside of PartialMerkleTree::CalcHash() (Chris Stewart) Pull request description: Fixing a possible memory access violation in CPartialMerkleTree::CalcHash(). This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them. This was found with #8469, specifically using this [generator](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/gen/merkleblock_gen.h#L52-L77) which will cause a memory access violation on [this test case](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/merkleblock_properties.cpp#L48). Tree-SHA512: b95904ec45ea3f082c7722161d93ee06b24c706fbffa909a6e995ed14788aed2830f91b626da6f0347660c45874a0735dab61c9440b59c949c690af4165c83fb

8276e70 Adding assert to avoid a memory access violation inside of PartialMerkleTree::CalcHash() (Chris Stewart) Pull request description: Fixing a possible memory access violation in CPartialMerkleTree::CalcHash(). This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them. This was found with bitcoin#8469, specifically using this [generator](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/gen/merkleblock_gen.h#L52-L77) which will cause a memory access violation on [this test case](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/merkleblock_properties.cpp#L48). Tree-SHA512: b95904ec45ea3f082c7722161d93ee06b24c706fbffa909a6e995ed14788aed2830f91b626da6f0347660c45874a0735dab61c9440b59c949c690af4165c83fb

laanwj added Bug Tests labels Mar 13, 2017

Christewart force-pushed the fix_mem_access_violation_merkleblock branch from 72bc779 to 51802cc Compare March 25, 2017 16:32

Christewart force-pushed the fix_mem_access_violation_merkleblock branch from 51802cc to bbdd771 Compare April 7, 2017 02:11

Christewart force-pushed the fix_mem_access_violation_merkleblock branch from bbdd771 to e3013b1 Compare July 12, 2017 14:34

TheBlueMatt reviewed Jul 12, 2017

View reviewed changes

Adding assert to avoid a memory access violation inside of PartialMer…

8276e70

…kleTree::CalcHash() Adding comment to assert in PartialMerkleTree::CalcHash() Adding comment on CMerkleBlock indicating it calls something that contains an assert Removing EOL whitespace

Christewart force-pushed the fix_mem_access_violation_merkleblock branch from e3013b1 to 8276e70 Compare July 12, 2017 15:49

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

sipa merged commit 8276e70 into bitcoin:master Jul 17, 2017

dagurval mentioned this pull request Jun 13, 2018

Merkleblock tests and refactor bitcoinxt/bitcoinxt#454

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix mem access violation merkleblock #9980

Fix mem access violation merkleblock #9980

Uh oh!

Conversation

Christewart commented Mar 12, 2017

Uh oh!

dcousens commented Mar 13, 2017

Uh oh!

pstratem commented Mar 14, 2017

Uh oh!

pstratem commented Mar 14, 2017

Uh oh!

pstratem commented Mar 14, 2017

Uh oh!

TheBlueMatt commented Mar 17, 2017

Uh oh!

Christewart commented Mar 26, 2017

Uh oh!

NicolasDorier commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Christewart commented Apr 7, 2017

Uh oh!

NicolasDorier commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 7, 2017

Uh oh!

TheBlueMatt commented Jul 11, 2017

Uh oh!

Christewart commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 17, 2017

Uh oh!

sipa commented Jul 17, 2017

Uh oh!

Uh oh!

NicolasDorier commented Mar 27, 2017 •

edited

Loading

jnewbery commented Apr 6, 2017 •

edited

Loading

NicolasDorier commented Apr 7, 2017 •

edited

Loading

Christewart commented Jul 12, 2017 •

edited

Loading