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 6228
Build 10503: Bitcoin ABC Buildbot (legacy)
Build 10502: 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

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

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

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

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

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

src/script/script_error.h
28

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

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