Parse(U)Int*
#32520
The legacy int parsing is problematic, because it accepts the + sign for unsigned integers. In all cases this is either:
+ sign was already rejected.+ sign is neither documented, nor can it be assumed to be present.Fix all issues by removing the legacy int parsing.
ToIntegral<uint32_t> only accepts numbers, so just use that to replace
the equivalent but more verbose way with find_first_not_of+ParseUInt32.
The unsigned version is never serialized with a `+` prefix, so the
stricter ToIntegral can be used.
It would be confusing to specify the sign for an unsigned value here, so
reject it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32520.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, brunoerg |
| Stale ACK | w0xlt, fjahr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
It would be confusing to specify the sign for an unsigned value here, so
reject it.
It does not make sense and it is rejected by other parsers as well:
>>> ipaddress.ip_network("1.2.3.0/+24")
ValueError: '1.2.3.0/+24' does not appear to be an IPv4 or IPv6 network
It is better to reject it with an error. For example,
$ bitcoin-cli -rpcconnect=127.0.0.1:+23501 -getinfo
error: Invalid port provided in -rpcconnect: 127.0.0.1:+23501
The + sign does not make sense for times or heights.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/42316044773
LLM reason (✨ experimental): The CI failure is due to Python linting errors identified by ruff.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
961@@ -962,8 +962,8 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
962 std::string height_str;
963 const RESTResponseFormat rf = ParseDataFormat(height_str, str_uri_part);
964
965- int32_t blockheight = -1; // Initialization done only to prevent valgrind false positive, see https://github.com/bitcoin/bitcoin/pull/18785
966- if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
967+ const auto blockheight{ToIntegral<int32_t>(height_str)};
968+ if (!blockheight || *blockheight < 0) {
969 return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
If we use SAFE_CHARS_URI here (seems appropriate since we’re dealing with a uri) the + sign is returned in the error description:
0diff --git a/src/rest.cpp b/src/rest.cpp
1index b340567bd1..464880224c 100644
2--- a/src/rest.cpp
3+++ b/src/rest.cpp
4@@ -964,7 +964,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
5
6 const auto blockheight{ToIntegral<int32_t>(height_str)};
7 if (!blockheight || *blockheight < 0) {
8- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
9+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str, SAFE_CHARS_URI));
10 }
11
12 CBlockIndex* pblockindex = nullptr;
13diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
14index ff76697884..a853b2ec0e 100755
15--- a/test/functional/interface_rest.py
16+++ b/test/functional/interface_rest.py
17@@ -272,7 +272,7 @@ class RESTTest (BitcoinTestFramework):
18 resp = self.test_rest_request(f"/blockhashbyheight/{INVALID_PARAM}", ret_type=RetType.OBJ, status=400)
19 assert_equal(resp.read().decode('utf-8').rstrip(), f"Invalid height: {INVALID_PARAM}")
20 resp = self.test_rest_request("/blockhashbyheight/+1", ret_type=RetType.OBJ, status=400)
21- assert_equal(resp.read().decode('utf-8').rstrip(), "Invalid height: 1")
22+ assert_equal(resp.read().decode('utf-8').rstrip(), "Invalid height: +1")
23 resp = self.test_rest_request("/blockhashbyheight/1000000", ret_type=RetType.OBJ, status=404)
24 assert_equal(resp.read().decode('utf-8').rstrip(), "Block height out of range")
25 resp = self.test_rest_request("/blockhashbyheight/-1", ret_type=RetType.OBJ, status=400)
228@@ -229,16 +229,15 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
229
230 static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
231 {
232- // parse requested index
233- int64_t inIdx = -1;
234- if (strInIdx != "" && (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size()))) {
235+ const auto idx{ToIntegral<int64_t>(strInIdx)};
nit: I think making this a size_t simplifies it a bit more?
0diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
1index 816f11045a..9e2c6d0902 100644
2--- a/src/bitcoin-tx.cpp
3+++ b/src/bitcoin-tx.cpp
4@@ -229,13 +229,13 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
5
6 static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
7 {
8- const auto idx{ToIntegral<int64_t>(strInIdx)};
9- if (strInIdx != "" && (!idx || *idx < 0 || *idx >= static_cast<int64_t>(tx.vin.size()))) {
10+ const auto idx{ToIntegral<size_t>(strInIdx)};
11+ if (strInIdx != "" && (!idx || *idx >= tx.vin.size())) {
12 throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
13 }
14
15 // set the nSequence to MAX_INT - 2 (= RBF opt in flag)
16- int cnt = 0;
17+ size_t cnt = 0;
18 for (CTxIn& txin : tx.vin) {
19 if (strInIdx == "" || cnt == *idx) {
20 if (txin.nSequence > MAX_BIP125_RBF_SEQUENCE) {
506@@ -507,26 +507,20 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
507
508 static void MutateTxDelInput(CMutableTransaction& tx, const std::string& strInIdx)
509 {
510- // parse requested deletion index
511- int64_t inIdx;
512- if (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size())) {
513+ const auto idx{ToIntegral<uint64_t>(strInIdx)};
tx.vin.size(), but using size_t seems more appropriate for a container index? (+ for MutateTxDelOutput)
13@@ -14,24 +14,9 @@ FUZZ_TARGET(parse_numbers)
14
15 (void)ParseMoney(random_string);
16
17- uint8_t u8;
18- (void)ParseUInt8(random_string, &u8);
ToIntegral fuzz test coverage. It’s a fairly simple function, so maybe that’s fine, but so was ParseIntegral. Reckon it’s worth keeping ToIntegral for the most common types in this harness?
ACK fa24012c181850d4adc5cca0fdba76d584ec3a00, nice cleanup.
Introduces some slight behaviour change when passing integer arguments through startup options, the REST interface, as and through bitcoin-cli and bitcoin-tx. While some of these changes may break existing user flows that were previously fine, they should hopefully be quite rare, and I think are worth the cleanup and more strict parsing.
When dealing with URI parts, it seems more consistent to use
corresponding SAFE_CHARS_URI mode in error messages.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
It would be confusing to specify the sign for an unsigned value here, so
reject it.
22+ const auto u64{ToIntegral<uint64_t>(random_string)};
23+ if (i8) {
24+ assert(i8 == i16);
25+ if (*i8 > 0) {
26+ assert(i8 == u8);
27+ }
nit: could assert that uints don’t underflow from --prefixed inputs?
0diff --git a/src/test/fuzz/parse_numbers.cpp b/src/test/fuzz/parse_numbers.cpp
1index b2465de517..21b1d534f2 100644
2--- a/src/test/fuzz/parse_numbers.cpp
3+++ b/src/test/fuzz/parse_numbers.cpp
4@@ -6,6 +6,9 @@
5 #include <util/moneystr.h>
6 #include <util/strencodings.h>
7
8+#include <cassert>
9+#include <cstdint>
10+#include <optional>
11 #include <string>
12
13 FUZZ_TARGET(parse_numbers)
14@@ -24,12 +27,16 @@ FUZZ_TARGET(parse_numbers)
15 assert(i8 == i16);
16 if (*i8 > 0) {
17 assert(i8 == u8);
18+ } else {
19+ assert(u8.value_or(0) == 0);
20 }
21 }
22 if (i16) {
23 assert(i16 == i32);
24 if (*i16 > 0) {
25 assert(i16 == u16);
26+ } else {
27+ assert(u16.value_or(0) == 0);
28 }
29 } else {
30 assert(!i8);
31@@ -38,6 +45,8 @@ FUZZ_TARGET(parse_numbers)
32 assert(i32 == i64);
33 if (*i32 > 0) {
34 assert(i32 == u32);
35+ } else {
36+ assert(u32.value_or(0) == 0);
37 }
38 } else {
39 assert(!i16);
40@@ -45,6 +54,8 @@ FUZZ_TARGET(parse_numbers)
41 if (i64) {
42 if (*i64 > 0) {
43 assert(i64 == u64);
44+ } else {
45+ assert(u64.value_or(0) == 0);
46 }
47 } else {
48 assert(!i32);
utACK 33332829333b589420f8038541d04ec6970f051d
I think I found two leftover mentions of the ParseInt in documentation: https://github.com/fjahr/bitcoin/commit/1f2f1a432b0fdb3d84eecc9dc3a2db34d88dc892 I can option a follow-up if you don’t want to address it here.
In the dev notes, remove the whole section, because:
* ParseDouble was removed in commit
fa9d72a7947d2cff541794e21e0040c3c1d43b32
* The locale-dependent atoi is already checked by
test/lint/lint-locale-dependence.py
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Did a grep to check if anything is missing, seems fine.