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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32520.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/42316044773</sub>
<sub>LLM reason (✨ experimental): The CI failure is due to Python linting errors identified by ruff.
</sub>
<details><summary>Hints</summary>
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.
</details>
Concept ACK
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:
<details> <summary>git diff on fa24012c18</summary>
diff --git a/src/rest.cpp b/src/rest.cpp
index b340567bd1..464880224c 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -964,7 +964,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
const auto blockheight{ToIntegral<int32_t>(height_str)};
if (!blockheight || *blockheight < 0) {
- return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str, SAFE_CHARS_URI));
}
CBlockIndex* pblockindex = nullptr;
diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
index ff76697884..a853b2ec0e 100755
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -272,7 +272,7 @@ class RESTTest (BitcoinTestFramework):
resp = self.test_rest_request(f"/blockhashbyheight/{INVALID_PARAM}", ret_type=RetType.OBJ, status=400)
assert_equal(resp.read().decode('utf-8').rstrip(), f"Invalid height: {INVALID_PARAM}")
resp = self.test_rest_request("/blockhashbyheight/+1", ret_type=RetType.OBJ, status=400)
- assert_equal(resp.read().decode('utf-8').rstrip(), "Invalid height: 1")
+ assert_equal(resp.read().decode('utf-8').rstrip(), "Invalid height: +1")
resp = self.test_rest_request("/blockhashbyheight/1000000", ret_type=RetType.OBJ, status=404)
assert_equal(resp.read().decode('utf-8').rstrip(), "Block height out of range")
resp = self.test_rest_request("/blockhashbyheight/-1", ret_type=RetType.OBJ, status=400)
</details>
thx, added a new commit on top with you as co-author
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?
<details> <summary>git diff on fa24012c18</summary>
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 816f11045a..9e2c6d0902 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -229,13 +229,13 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
{
- const auto idx{ToIntegral<int64_t>(strInIdx)};
- if (strInIdx != "" && (!idx || *idx < 0 || *idx >= static_cast<int64_t>(tx.vin.size()))) {
+ const auto idx{ToIntegral<size_t>(strInIdx)};
+ if (strInIdx != "" && (!idx || *idx >= tx.vin.size())) {
throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
}
// set the nSequence to MAX_INT - 2 (= RBF opt in flag)
- int cnt = 0;
+ size_t cnt = 0;
for (CTxIn& txin : tx.vin) {
if (strInIdx == "" || cnt == *idx) {
if (txin.nSequence > MAX_BIP125_RBF_SEQUENCE) {
</details>
consensus rules aren't architecture or compiler-dependent, so I used uint32_t instead
Ah, good point, that seems sensible. (+ for other)
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)};
nit: looks safe because we're comparing with tx.vin.size(), but using size_t seems more appropriate for a container index? (+ for MutateTxDelOutput)
(same)
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);
It seems we now no longer have 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?
thx, added some fuzz
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.
Concept ACK
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?
<details> <summary>git diff on fa2445eea3</summary>
diff --git a/src/test/fuzz/parse_numbers.cpp b/src/test/fuzz/parse_numbers.cpp
index b2465de517..21b1d534f2 100644
--- a/src/test/fuzz/parse_numbers.cpp
+++ b/src/test/fuzz/parse_numbers.cpp
@@ -6,6 +6,9 @@
#include <util/moneystr.h>
#include <util/strencodings.h>
+#include <cassert>
+#include <cstdint>
+#include <optional>
#include <string>
FUZZ_TARGET(parse_numbers)
@@ -24,12 +27,16 @@ FUZZ_TARGET(parse_numbers)
assert(i8 == i16);
if (*i8 > 0) {
assert(i8 == u8);
+ } else {
+ assert(u8.value_or(0) == 0);
}
}
if (i16) {
assert(i16 == i32);
if (*i16 > 0) {
assert(i16 == u16);
+ } else {
+ assert(u16.value_or(0) == 0);
}
} else {
assert(!i8);
@@ -38,6 +45,8 @@ FUZZ_TARGET(parse_numbers)
assert(i32 == i64);
if (*i32 > 0) {
assert(i32 == u32);
+ } else {
+ assert(u32.value_or(0) == 0);
}
} else {
assert(!i16);
@@ -45,6 +54,8 @@ FUZZ_TARGET(parse_numbers)
if (i64) {
if (*i64 > 0) {
assert(i64 == u64);
+ } else {
+ assert(u64.value_or(0) == 0);
}
} else {
assert(!i32);
</details>
thx. I don't want to check any explicit values, so I pushed something else.
re-ACK fa2445eea3e380386ab01631b735c7eeb6b76b58
re-ACK 33332829333b589420f8038541d04ec6970f051d
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>
thx, I pushed a commit similar to yours. (Generally, when it comes to the dev notes, I think it is clear that no one is reading them, because of all the stale references in it. We should probably work on reducing them to only contain the important stuff that is not already checked by the automated linters or not already covered by other docs)
re-ACK faf55fc80b
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Did a grep to check if anything is missing, seems fine.