From fad55e79ca18a5894a8da6db6309c323eecbb178 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 5 Oct 2021 13:34:24 +0200 Subject: [PATCH 1/4] doc: Fixup ToIntegral docs --- src/util/strencodings.h | 4 ++-- test/lint/lint-locale-dependence.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/strencodings.h b/src/util/strencodings.h index 1f7762aeef8..eedb5ec2f86 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -72,7 +72,7 @@ void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut); // LocaleIndependentAtoi is provided for backwards compatibility reasons. // -// New code should use the ParseInt64/ParseUInt64/ParseInt32/ParseUInt32 functions +// New code should use ToIntegral or the ParseInt* functions // which provide parse error feedback. // // The goal of LocaleIndependentAtoi is to replicate the exact defined behaviour @@ -125,7 +125,7 @@ constexpr inline bool IsSpace(char c) noexcept { /** * Convert string to integral type T. Leading whitespace, a leading +, or any * trailing character fail the parsing. The required format expressed as regex - * is `-?[0-9]+`. + * is `-?[0-9]+`. The minus sign is only permitted for signed integer types. * * @returns std::nullopt if the entire string could not be parsed, or if the * parsed value is not in the range representable by the type T. diff --git a/test/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh index 3015c4f9b94..4a3eefa74c3 100755 --- a/test/lint/lint-locale-dependence.sh +++ b/test/lint/lint-locale-dependence.sh @@ -38,7 +38,7 @@ export LC_ALL=C # https://stackoverflow.com/a/34878283 for more details. # TODO: Reduce KNOWN_VIOLATIONS by replacing uses of locale dependent stoul/strtol with locale -# independent ToIntegral(...). +# independent ToIntegral(...) or the ParseInt*() functions. # TODO: Reduce KNOWN_VIOLATIONS by replacing uses of locale dependent snprintf with strprintf. KNOWN_VIOLATIONS=( "src/bitcoin-tx.cpp.*stoul" From fa03dec7e98bdda8aa596ef7943cf0a8d0bcb127 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 8 Oct 2021 15:39:46 +0200 Subject: [PATCH 2/4] refactor: Use C++11 range based for loop in ParseScript --- src/core_read.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index 320811b9e98..5effd491515 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -59,17 +59,17 @@ CScript ParseScript(const std::string& s) std::vector words; boost::algorithm::split(words, s, boost::algorithm::is_any_of(" \t\n"), boost::algorithm::token_compress_on); - for (std::vector::const_iterator w = words.begin(); w != words.end(); ++w) + for (const std::string& w : words) { - if (w->empty()) + if (w.empty()) { // Empty string, ignore. (boost::split given '' will return one word) } - else if (std::all_of(w->begin(), w->end(), ::IsDigit) || - (w->front() == '-' && w->size() > 1 && std::all_of(w->begin()+1, w->end(), ::IsDigit))) + else if (std::all_of(w.begin(), w.end(), ::IsDigit) || + (w.front() == '-' && w.size() > 1 && std::all_of(w.begin()+1, w.end(), ::IsDigit))) { // Number - int64_t n = LocaleIndependentAtoi(*w); + int64_t n = LocaleIndependentAtoi(w); //limit the range of numbers ParseScript accepts in decimal //since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts @@ -80,23 +80,23 @@ CScript ParseScript(const std::string& s) result << n; } - else if (w->substr(0,2) == "0x" && w->size() > 2 && IsHex(std::string(w->begin()+2, w->end()))) + else if (w.substr(0,2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin()+2, w.end()))) { // Raw hex data, inserted NOT pushed onto stack: - std::vector raw = ParseHex(std::string(w->begin()+2, w->end())); + std::vector raw = ParseHex(std::string(w.begin()+2, w.end())); result.insert(result.end(), raw.begin(), raw.end()); } - else if (w->size() >= 2 && w->front() == '\'' && w->back() == '\'') + else if (w.size() >= 2 && w.front() == '\'' && w.back() == '\'') { // Single-quoted string, pushed as data. NOTE: this is poor-man's // parsing, spaces/tabs/newlines in single-quoted strings won't work. - std::vector value(w->begin()+1, w->end()-1); + std::vector value(w.begin()+1, w.end()-1); result << value; } else { // opcode, e.g. OP_ADD or ADD: - result << ParseOpCode(*w); + result << ParseOpCode(w); } } From fa053c0019bc8b2174c485f4885f894f2b5de472 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 8 Oct 2021 15:39:19 +0200 Subject: [PATCH 3/4] style: Fix whitespace in Parse* functions --- src/core_read.cpp | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index 5effd491515..3bbecbf52bd 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -26,20 +26,20 @@ opcodetype ParseOpCode(const std::string& s) { static std::map mapOpNames; - if (mapOpNames.empty()) - { - for (unsigned int op = 0; op <= MAX_OPCODE; op++) - { + if (mapOpNames.empty()) { + for (unsigned int op = 0; op <= MAX_OPCODE; op++) { // Allow OP_RESERVED to get into mapOpNames - if (op < OP_NOP && op != OP_RESERVED) + if (op < OP_NOP && op != OP_RESERVED) { continue; + } std::string strName = GetOpName(static_cast(op)); - if (strName == "OP_UNKNOWN") + if (strName == "OP_UNKNOWN") { continue; + } mapOpNames[strName] = static_cast(op); // Convenience: OP_ADD and just ADD are both recognized: - if (strName.compare(0, 3, "OP_") == 0) { // strName starts with "OP_" + if (strName.compare(0, 3, "OP_") == 0) { // strName starts with "OP_" mapOpNames[strName.substr(3)] = static_cast(op); } } @@ -59,42 +59,33 @@ CScript ParseScript(const std::string& s) std::vector words; boost::algorithm::split(words, s, boost::algorithm::is_any_of(" \t\n"), boost::algorithm::token_compress_on); - for (const std::string& w : words) - { - if (w.empty()) - { + for (const std::string& w : words) { + if (w.empty()) { // Empty string, ignore. (boost::split given '' will return one word) - } - else if (std::all_of(w.begin(), w.end(), ::IsDigit) || - (w.front() == '-' && w.size() > 1 && std::all_of(w.begin()+1, w.end(), ::IsDigit))) + } else if (std::all_of(w.begin(), w.end(), ::IsDigit) || + (w.front() == '-' && w.size() > 1 && std::all_of(w.begin() + 1, w.end(), ::IsDigit))) { // Number int64_t n = LocaleIndependentAtoi(w); - //limit the range of numbers ParseScript accepts in decimal - //since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts + // limit the range of numbers ParseScript accepts in decimal + // since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts if (n > int64_t{0xffffffff} || n < -1 * int64_t{0xffffffff}) { throw std::runtime_error("script parse error: decimal numeric value only allowed in the " "range -0xFFFFFFFF...0xFFFFFFFF"); } result << n; - } - else if (w.substr(0,2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin()+2, w.end()))) - { + } else if (w.substr(0, 2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))) { // Raw hex data, inserted NOT pushed onto stack: - std::vector raw = ParseHex(std::string(w.begin()+2, w.end())); + std::vector raw = ParseHex(std::string(w.begin() + 2, w.end())); result.insert(result.end(), raw.begin(), raw.end()); - } - else if (w.size() >= 2 && w.front() == '\'' && w.back() == '\'') - { + } else if (w.size() >= 2 && w.front() == '\'' && w.back() == '\'') { // Single-quoted string, pushed as data. NOTE: this is poor-man's // parsing, spaces/tabs/newlines in single-quoted strings won't work. - std::vector value(w.begin()+1, w.end()-1); + std::vector value(w.begin() + 1, w.end() - 1); result << value; - } - else - { + } else { // opcode, e.g. OP_ADD or ADD: result << ParseOpCode(w); } From fa43e7c2d9dc5e2df70acd2019bdd24023c1d333 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 8 Oct 2021 15:51:55 +0200 Subject: [PATCH 4/4] bitcoin-tx: Avoid treating overflow as OP_0 --- src/core_read.cpp | 6 +++--- src/test/script_parse_tests.cpp | 2 +- test/util/data/bitcoin-util-test.json | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index 3bbecbf52bd..2149b428d26 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -66,16 +66,16 @@ CScript ParseScript(const std::string& s) (w.front() == '-' && w.size() > 1 && std::all_of(w.begin() + 1, w.end(), ::IsDigit))) { // Number - int64_t n = LocaleIndependentAtoi(w); + const auto num{ToIntegral(w)}; // limit the range of numbers ParseScript accepts in decimal // since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts - if (n > int64_t{0xffffffff} || n < -1 * int64_t{0xffffffff}) { + if (!num.has_value() || num > int64_t{0xffffffff} || num < -1 * int64_t{0xffffffff}) { throw std::runtime_error("script parse error: decimal numeric value only allowed in the " "range -0xFFFFFFFF...0xFFFFFFFF"); } - result << n; + result << num.value(); } else if (w.substr(0, 2) == "0x" && w.size() > 2 && IsHex(std::string(w.begin() + 2, w.end()))) { // Raw hex data, inserted NOT pushed onto stack: std::vector raw = ParseHex(std::string(w.begin() + 2, w.end())); diff --git a/src/test/script_parse_tests.cpp b/src/test/script_parse_tests.cpp index 5b8b6a725f7..004c1a9a84f 100644 --- a/src/test/script_parse_tests.cpp +++ b/src/test/script_parse_tests.cpp @@ -38,7 +38,6 @@ BOOST_AUTO_TEST_CASE(parse_script) {"'17'", "023137"}, {"ELSE", "67"}, {"NOP10", "b9"}, - {"11111111111111111111", "00"}, }; std::string all_in; std::string all_out; @@ -49,6 +48,7 @@ BOOST_AUTO_TEST_CASE(parse_script) } BOOST_CHECK_EQUAL(HexStr(ParseScript(all_in)), all_out); + BOOST_CHECK_EXCEPTION(ParseScript("11111111111111111111"), std::runtime_error, HasReason("script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF")); BOOST_CHECK_EXCEPTION(ParseScript("11111111111"), std::runtime_error, HasReason("script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF")); BOOST_CHECK_EXCEPTION(ParseScript("OP_CHECKSIGADD"), std::runtime_error, HasReason("script parse error: unknown opcode")); } diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json index a648c0287ab..20684f0f76c 100644 --- a/test/util/data/bitcoin-util-test.json +++ b/test/util/data/bitcoin-util-test.json @@ -294,6 +294,12 @@ "output_cmp": "txcreatescript4.json", "description": "Create a new transaction with a single output script (OP_DROP) in a P2SH, wrapped in a P2SH (output as json)" }, + { "exec": "./bitcoin-tx", + "args": ["-create", "outscript=0:999999999999999999999999999999"], + "return_code": 1, + "error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF", + "description": "Try to parse an output script with a decimal number above the allowed range" + }, { "exec": "./bitcoin-tx", "args": ["-create", "outscript=0:9999999999"], "return_code": 1,