Page MenuHomePhabricator

check negative CScriptNum.getint() and introduce new error code
AbandonedPublic

Authored by markblundeberg on Jun 8 2019, 23:39.

Details

Reviewers
deadalnix
jasonbcox
Mengerian
Group Reviewers
Restricted Project
Summary

signed int -> uint64_t is a bit of a weird implicit cast, given that
scripts can provide negative numbers, and .getint() uses clipping that
depends on platform's int size.

This Diff gives a more informative error message when NUM2BIN size is negative,
instead of complaining about too large pushes. The negative number error code
will be used for multisig schnorr too.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
improve_getint
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6227
Build 10501: Bitcoin ABC Buildbot (legacy)
Build 10500: arc lint + arc unit

Event Timeline

rebase just to get another build (mystery teamcity fail)

*sigh* this works on my local machine, I swear! =D

markblundeberg edited the summary of this revision. (Show Details)

oops, missed the json tests somehow

src/script/interpreter.cpp
1128 ↗(On Diff #9266)

note -- the (int) cast style is in line with other opcodes like OP_ROLL, OP_CHECKMULTISIG

deadalnix requested changes to this revision.Jun 12 2019, 17:12

Please don't add more assumptions into the code. Also please explain why this is needed for schnorr multisig.

src/script/interpreter.cpp
1128 ↗(On Diff #9266)

You added the assumption that data.size() fits into an int. Which it does, but regardless, baking more assumptions into the code is a very clear regression.

1153 ↗(On Diff #9266)

Why don't you get the size as an int and then test that ? You can still have both checks and a nicer error code this way.

1160 ↗(On Diff #9266)

Even though it is unlikely to matter in practice, it is a good idea to have one check for error condition then detail in there.

if (uint64_t(size) > MAX_SCRIPT_ELEMENT_SIZE) {
  return set_error(serror, size < 0 ? SCRIPT_ERR_NEGATIVE_NUMBER : SCRIPT_ERR_PUSH_SIZE);
}
src/script/script_error.cpp
49 ↗(On Diff #9266)

This is not an error in itself. This needs to be reformulated.

src/script/script_error.h
28 ↗(On Diff #9266)

Maybe unexpected negative number ? Because negative numbers are not errors per se.

This revision now requires changes to proceed.Jun 12 2019, 17:12