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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 6228
Build 10503: Bitcoin ABC Teamcity Staging
Build 10502: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jun 8 2019, 23:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 8 2019, 23:39
markblundeberg updated this revision to Diff 9265.Jun 8 2019, 23:50

rebase just to get another build (mystery teamcity fail)

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

markblundeberg planned changes to this revision.Jun 9 2019, 00:02
markblundeberg updated this revision to Diff 9266.Jun 9 2019, 00:19
markblundeberg edited the summary of this revision. (Show Details)

oops, missed the json tests somehow

markblundeberg added inline comments.Jun 9 2019, 00:23
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
markblundeberg abandoned this revision.Jul 10 2019, 16:08