Following up on this suggestion : #18413 (comment), prevent the output of atoi64 in the core_read.cpp:ParseScript helper to send to CScriptNum::serialize values wider than 32-bit.
Since the ParseScript helper is only used by the tool defined in bitcoin-tx.cpp, this only prevents users to provide too much unrealistic values.
66 | + //for details, see : https://github.com/bitcoin/bitcoin/pull/18413#issuecomment-602954469 67 | + if(n > std::numeric_limits<int32_t>::max()) { 68 | + n = std::numeric_limits<int32_t>::max(); 69 | + } else if(n < std::numeric_limits<int32_t>::min()) { 70 | + n = std::numeric_limits<int32_t>::min(); 71 | + }
FWIW, here's an alternative shorter version for the same logic, without branches:
n = std::min(n, static_cast<int64_t>(std::numeric_limits<int32_t>::max()));
n = std::max(n, static_cast<int64_t>(std::numeric_limits<int32_t>::min()));
Not sure if it's really more readable though, personally I'd be okay with both versions.
std::clamp in C++17 would be even better and less verbose, but unsure if we can use C++17 features ?
Note that min/max in the standard also do exactly one comparison each.
I can remove the if/else though if people think it fits better here.
Ah, std::clamp would indeed be a very nice option. Unfortunately, upgrading to C++17 has not happened yet (see this discussion: #16684), so it can't be used.
Sometimes substitutes for not-yet-available C++ features are implemented by contributors, like Span for the C++20 std::span by sipa (see #12886), don't know though if that is a desired way to go here, and if yes, if it should be packed in this PR. This should probably include identifiying all clamps throughout the codebase and using our newly introduced clamp function (so it can be replaced with std::clamp once we move to C++17).
Thanks, I'd be down to do that in a different PR. I'm looking for stuffs to do to improve my knowledge of the codebase anyway :)
I'll wait for some other people to confirm "yes this is a good idea, do a separate PR for std::clamp" before starting though
It seems that the tests were already including values wider than 2^32 so I reckon there is 2 options :
The second option seemed the less intrusive, so I updated the PR to only forbid the value -2^63. I also added 2 tests reflecting that : before this PR, those tests were triggering the UB before failing.
I think we should just fix UB in CScriptNum::serialize; that should be easy, and avoid all issues.
In addition, I think we may want to restrict what range of numbers ParseScript accepts in decimal, because numbers outside -0xFFFFFFFF...0xFFFFFFFF are simply illegal in scripts (they don't work as numbers, so they should really be represented as hex strings instead if they're used at all). That has nothing to do with avoiding UB; it's just fixing weird behavior in ParseScript.
OK, thanks.
So. if I understand correctly, we do agree that the following tests in src/test/data/script_tests.json are wrong and should be removed (most of them originally from 90320d67779be5c97061380c035d3fe51b7ce74b ) ?
["549755813887", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["549755813888", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["9223372036854775807", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813887", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813888", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["-9223372036854775807", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["549755813887", "0x05 0xFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["549755813888", "0x06 0x000000008000 EQUAL", "P2SH,STRICTENC", "OK"],
["9223372036854775807", "0x08 0xFFFFFFFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813887", "0x05 0xFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813888", "0x06 0x000000008080 EQUAL", "P2SH,STRICTENC", "OK"],
["-9223372036854775807", "0x08 0xFFFFFFFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["4294967296", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", "CSV fails if stack top bit 1 << 31 is not set, and tx version < 2"],
Ow my bad, I was too focused on the atoi64 branch to see it that way, thanks.
I edited my branch accordingly.
173 | @@ -174,7 +174,7 @@ 174 | "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ff64cd1d", "P2SH,CHECKLOCKTIMEVERIFY"], 175 | 176 | ["Argument 2^32 with nLockTime=2^32-1"], 177 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 CHECKLOCKTIMEVERIFY 1"]],
Shouldn't this version be kept as an example of an _invalid script?
OK, thanks. This is a good idea I think, I was hesitant to add more tests. If I understood you correctly, the tests are invalids here, so I guess you mean adding something like:
["Argument 2^32 in decimal (hence clamped to 0xffffffff) with nLockTime=2^32-2"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 CHECKLOCKTIMEVERIFY 1"]],
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000fffffffe", "P2SH,CHECKLOCKTIMEVERIFY"],
If this is correct, I guess I should add similar tests for other modified test cases too ?
Feel free to add as many tests as you like, as long as all existing proper test cases also stay there
OK great, I added 5 more tests in the branch.
What I mean is that this shouldn't be silently clamped. The test here looks right. Why would it be safe to parse a script and silently change it while parsing?
OK, so you are suggesting in the core_read.cpp above to return an std::runtime_error when the result of the atoi64 is beyond the the range -0xffffffff..0xffffffff instead ?
Yes, some kind of failure
std::runtime_error is thrown on other script parse errors in ParseScript(…). Shouldn't that be thrown also for this type of script parse error in ParseScript?
OK, thanks, this is clearly a possibility.
Note that for now, no test trigger the already existing std::runtime_error. script_json_test doesn't handle exceptions, so it should be modified to catch them (maybe in a different PR then ?). Testcases will have to be modified to handle the exception too I guess.
(the fuzzer harness already handle exceptions well)
I'm not sure which possibility (clamping or throwing an error) fits more the style of the codebase. Maybe getting an ACK from @sipa for the exception first ?
@practicalswift hm sorry but I guess it's better to ask first: if you return an std::runtime_error, how to describe the error in the JSON test file ?
I guess that adding a new value to the ScriptError_t enum is too intrusive.
Adding a special if/else case in script_test.cpp:ParseScriptError seems too hackish.
Reusing an existing ScriptError_t seems the less odd option - something like SCRIPT_ERR_BAD_OPCODE or SCRIPT_ERR_OP_COUNT ?
I don't think there is a need. (tx/script)_(valid/invalid) are testing the transaction and script validity logic - not the script parser.
If a particular test now causes an exception to be thrown, just write that test differently. It's still testing the same thing.
OK, so as stated below, if a test contains 4294967296 CHECKLOCKTIMEVERIFY 1 or 999...999, this is a script parser error which should be covered in bitcoin-util-test.json, not here. I believe this resolve this issue ?
What's the rationale here for saturating the number instead of raising an error? I generally prefer explicit parse errors on invalid input, at least.
@laanwj Agreed. That would also be in line with how parsing errors are handled elsewhere in ParseScript(…): a std::runtime_error is thrown :)
Yes, that's what I meant to suggest: fail if a number outside the valid range is present. Sorry if that was not clear.
Slightly related: The fuzz tests already have the error condition in this parser here covered: https://marcofalke.github.io/btc_cov/fuzz.coverage/src/core_read.cpp.gcov.html#84 . However our unit and functional tests don't know how to handle this/have this uncovered: https://marcofalke.github.io/btc_cov/total.coverage/src/core_read.cpp.gcov.html#84
It looks like a test case can be added to:
src/test/script_tests.cpp and/ortest/util/data/bitcoin-util-test.json with binary bitcoin-tx
The exception is simply passed in an error_text in test/util/data/bitcoin-util-test.json. E.g.
{ "exec": "./bitcoin-tx",
"args": ["-create", "nversion=1foo"],
"return_code": 1,
"error_txt": "error: Invalid TX version requested",
"description": "Tests the check for invalid nversion value"
},
Ow, OK, thanks, I see.
So this way if we return an exception, we can just add a test ./src/bitcoin-tx -create outscript=0:9999999999 to check the parser (and have code coverage for the added exception) - hence the test in src/test/data/tx_invalid.json containing 4294967296 CHECKLOCKTIMEVERIFY 1 becomes useless since it would just trigger the same exception in the parser.
297 | "020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffbf7f0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"], 298 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "6442450944 CHECKSEQUENCEVERIFY 1"]], 299 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000008001 CHECKSEQUENCEVERIFY 1"]], 300 | "020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffff7f0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"], 301 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "6442450944 CHECKSEQUENCEVERIFY 1"]], 302 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000008001 CHECKSEQUENCEVERIFY 1"]],
Note that using an exception allowed to raise new errors in tx_valid - before the clamped value also valided the test...
59 | @@ -59,6 +60,14 @@ CScript ParseScript(const std::string& s) 60 | { 61 | // Number 62 | int64_t n = atoi64(*w); 63 | + 64 | + //limit the range of numbers ParseScript accepts in decimal 65 | + //since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts 66 | + if(n > static_cast<int64_t>(0xffffffff) || n < -1*static_cast<int64_t>(0xffffffff)) {
Any reason this needs a static cast?
if (n > 0xffffffff || n < -0xffffffff) {
The second static_cast seems strictly necessary, see this example : https://godbolt.org/z/Ko33NG
The first one is superficial (implicit), I just thought that the symetry looked better. I can remove the first one if you prefer ?
edit : at least fixed whitespace style
edit 2 : also note that the LL suffix could be used but this would assume that long long = 64bits
Explicit is better than implicit :)
I think with C++11 you can also get a safer way to achieve the same: int64_t{bla}. If the value doesn't fit in the integer type, it won't compile. Whereas with static_cast it would compile and give the wrong result.
Thanks, updated the branch accordingly,
OK, I think I updated the branch as we discussed above ?
If this PR is OK, I'll add a test case in bitcoin-util-test.json for the second exception in ParseScript in another PR.
267 | + { "exec": "./bitcoin-tx", 268 | + "args": ["-create", "outscript=0:-9999999999"], 269 | + "return_code": 1, 270 | + "error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF", 271 | + "description": "Try to parse an output script with a decimal number below the allowed range" 272 | + },
Could also check that +-0xffffffff is accepted and +-(0xffffffff+1) is not?
Yes, thanks, great idea.
Concept ACK
ACK 4cefaace020f406d1143cf1351988958ef360861
(absorbing an empty commit to re-launch failed CI)
re-ACK 2ee8cc07292e62a1be47d09421f93d25ad24fbd4
18 | @@ -19,6 +19,7 @@ 19 | #include <boost/algorithm/string/split.hpp> 20 | 21 | #include <algorithm> 22 | +#include <limits>
Is <limits> still necessary?
Ow, thank you. No it isn't, just removed it.
ACK 9ab14e4d21c73d16d8d782f1576fe29e659e2a70 (only difference is removing the limits include as suggested: https://github.com/bitcoin/bitcoin/compare/2ee8cc07292e62a1be47d09421f93d25ad24fbd4..9ab14e4d21c73d16d8d782f1576fe29e659e2a70)
Thanks !
I'm very sorry to only realize this now but atoi64 itself doesn't do bounds checking. So very large or small values outside the 64 bit range will still overflow the 64 bit or at least not raise an error conditions.
I think the appropriate function to use here is our own ParseUInt32.
Sorry, I don't follow, atoi64 is:
int64_t atoi64(const std::string& str)
{
#ifdef _MSC_VER
return _atoi64(str.c_str());
#else
return strtoll(str.c_str(), nullptr, 10);
#endif
}
And both _atoi64 and strtoll both return LLONG_MAX or LLONG_MIN when receiving too big positive/negative values ?
It will additionally set an errno but since we check beforehand that we are parsing a number, there should not be any issue ?
In anycase it's true that ParseInt32 (since the value can be signed) could be more adapted since it checks for errno anyway. I could open a new PR for that if people prefer ?
And both _atoi64 and strtoll both return LLONG_MAX or LLONG_MIN when receiving too big positive/negative values ?
If that's the case this is actually OK. I thought they didn't have any guaranteed return value in that case.
The strtol() function returns the result of the conversion, unless the value would underflow or overflow. If an underflow occurs, strtol() returns LONG_MIN. If an overflow occurs, str‐
tol() returns LONG_MAX.
both LONG_MIN and LONG_MAX are out of the range so it will do so correctly…
In anycase it's true that ParseInt32 (since the value can be signed) could be more adapted since it checks for errno anyway. I could open a new PR for that if people prefer ?
Oh right! then you'd actually need ParseInt64: the value can be signed, and it's not in the range of a 32 bit signed integer (-0x80000000…0x7fffffff). But I think it's fine to keep it as-is then, phew.
OK, great, thanks for the code review !
2520 | @@ -2521,7 +2521,7 @@ 2521 | ["-1", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "NEGATIVE_LOCKTIME", "CSV automatically fails if stack top is negative"], 2522 | ["0x0100", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY,MINIMALDATA", "UNKNOWN_ERROR", "CSV fails if stack top is not minimally encoded"], 2523 | ["0", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", "CSV fails if stack top bit 1 << 31 is set and the tx version < 2"], 2524 | -["4294967296", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", 2525 | +["0x050000000001", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME",
Why are you changing all the values when translating from dec to hex? It might be fine, but at least for locktime values there are different code paths depending on the value.
Ow, good point. I double checked with the value returned by ParseScript in the tests before/after the PR - so that the behavior of the tests doesn't change at all (i.e. before this PR the script built by ParseScript from 4294967296 contained 0x050000000001)
Ah, oops. My bad