Add basic sanity checking for script pushdata size
ClosedPublic
Actions

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

Details

Reviewers

jasonbcox
schancel

Group Reviewers

Restricted Project

Commits

rSTAGINGbfde70fb948d: Add basic sanity checking for script pushdata size
rABCbfde70fb948d: Add basic sanity checking for script pushdata size

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 2330
Build 2797: Bitcoin ABC Buildbot (legacy)
Build 2796: arc lint + arc unit

Event Timeline

deadalnix created this revision.Apr 16 2018, 21:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 16 2018, 21:44

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Thanks for the quick fix.

This revision is now accepted and ready to land.Apr 16 2018, 21:50

deadalnix mentioned this in T327: Ensure pushdata size are enforced by ParseScript.Apr 16 2018, 21:52

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2325: Diff 3518.Apr 16 2018, 22:02

jimpo added a subscriber: jimpo.Apr 16 2018, 22:17

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 a subscriber: schancel.Apr 16 2018, 22:57

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

Rebase on top of D1290

deadalnix added inline comments.Apr 17 2018, 01:00

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.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster completed remote builds in B2330: Diff 3525.Apr 17 2018, 02:24

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

schancel added a child revision: D1295: Ensure checks are enforced in core_read.cpp for all push types.Apr 17 2018, 05:26

schancel added inline comments.Apr 17 2018, 17:02

src/core_read.cpp
52	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