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
Branch
checkscriptpush
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2325
Build 2787: Bitcoin ABC Buildbot (legacy)
Build 2786: arc lint + arc unit

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

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

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

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.