Page MenuHomePhabricator

fix a deserialization overflow edge case

Authored by Fabien on Jan 23 2020, 10:54.


A specially-constructed BlockTransactionsRequest can cause offset to
wrap in deserialization. In the current code, there is not any way this
could be dangerous; but disallowing it reduces the potential for future

Backport of core PR14685.

The bug this PR is fixing doesn't apply directly to our code, since we
are already using 32 bit indexes (renames indices) since D1146 and the
deserialization size is limited to 0x02000000 by the MAX_SIZE constant
(see, so
MAX_SIZE < max(uint32_t) is the limiting factor for us.
In the meantime the overflow issue will occur if the MAX_SIZE constant
is raised above max(uint32_t).

This diff is also adapted to test the BlockTransactionRequest
deserialization against MAX_SIZE and make sure the overflow edge-case
is un der control should the constant get raised. Static asserts will
cause the build to fail if the tests assumptions are no longer valid.

Test Plan
ninja all check
./test/functional/ abc-p2p-compactblocks

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
434 ↗(On Diff #15755)

Nit: the plurality here is weird. should be indexType or indicesType, of which the latter is probably more correct.

436 ↗(On Diff #15755)

This isn't the first place I would put this check (somewhere near the definition seems more natural to me), but I'm not able to put my finger on a strong objective reasoning for this.

466 ↗(On Diff #15755)


486 ↗(On Diff #15755)

Macro likestamp:

This revision is now accepted and ready to land.Jan 24 2020, 18:27
434 ↗(On Diff #15755)

This is the type of an element of the vector of indices, so I think indiceType is correct.

436 ↗(On Diff #15755)

I'm not sure this assertion should hold as a general case. If one day we raise MAX_SIZE to something larger, I'm make sure that this test will fail and will need to be updated. I'm not sure that the code will fail, only this test (and eventually the next one).

466 ↗(On Diff #15755)

Ditto :)