Page MenuHomePhabricator

Add basic sanity checking for script pushdata size
ClosedPublic

Authored by deadalnix on Apr 16 2018, 21:44.

Details

Summary

This allowed to uncover several errors in the json test suite.

The checking right now is rather basic, but should catch a good chunk of errors. Actually checking for size when faced with pushdata and when pushing integer literals and/or opcodes. Convertly, we should check that no push is present when no push is expected.

Depends on D1292, Relates to T327

Test Plan
make check

Added a test for ParseScript. It's barebone for now, but at least cover the functionality added.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jasonbcox added a subscriber: jasonbcox.

Thanks for the quick fix.

This revision is now accepted and ready to land.Apr 16 2018, 21:50
jimpo added inline comments.
src/test/core_io_tests.cpp
16 ↗(On Diff #3518)

This is the same as line 14.

Worth adding a case for ParseScript("0x00 0P_1") as well probably (or 0x00 followed by anything valid).

schancel added inline comments.
src/core_read.cpp
117 ↗(On Diff #3518)

This is wrong. OP_PUSHDATA1-4 get's the next push size from the next N bytes. You need a lookahead here or another value indicated how many bytes the next_push_size will be stored in.

schancel requested changes to this revision.Apr 16 2018, 22:59
This revision now requires changes to proceed.Apr 16 2018, 22:59
src/core_read.cpp
117 ↗(On Diff #3518)

So, as a result, what comes next is a buffer of 1, 2 or 4 bytes representing the size. The code is correct - or it would break every single test using pushdata.

src/core_read.cpp
52 ↗(On Diff #3525)

Can we please decide on a style. I think Microsoft's general guidelines are great:

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions

This revision is now accepted and ready to land.Apr 17 2018, 17:46

Check push size for all kind of things rather than just hex buffers.

This revision was automatically updated to reflect the committed changes.