Skip to content

Commit 290ff5e

Browse files
author
MarcoFalke
committed
Merge bitcoin/bitcoin#24041: util: Restore GetIntArg saturating behavior
b5c9bb5 util: Restore GetIntArg saturating behavior (James O'Beirne) Pull request description: The new locale-independent atoi64 method introduced in #20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions. Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before #20452. Then #20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again. This change is a stripped-down alternative version of #23841 by jamesob ACKs for top commit: jamesob: Github ACK bitcoin/bitcoin@b5c9bb5 vincenzopalazzo: ACK bitcoin/bitcoin@b5c9bb5 MarcoFalke: review ACK b5c9bb5 🌘 Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae
2 parents 16781e1 + b5c9bb5 commit 290ff5e

File tree

5 files changed

+66
-25
lines changed

5 files changed

+66
-25
lines changed

‎src/test/fuzz/string.cpp

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -276,20 +276,14 @@ FUZZ_TARGET(string)
276276
}
277277

278278
{
279-
const int atoi_result = atoi(random_string_1.c_str());
280279
const int locale_independent_atoi_result = LocaleIndependentAtoi<int>(random_string_1);
281280
const int64_t atoi64_result = atoi64_legacy(random_string_1);
282-
const bool out_of_range = atoi64_result < std::numeric_limits<int>::min() || atoi64_result > std::numeric_limits<int>::max();
283-
if (out_of_range) {
284-
assert(locale_independent_atoi_result == 0);
285-
} else {
286-
assert(atoi_result == locale_independent_atoi_result);
287-
}
281+
assert(locale_independent_atoi_result == std::clamp<int64_t>(atoi64_result, std::numeric_limits<int>::min(), std::numeric_limits<int>::max()));
288282
}
289283

290284
{
291285
const int64_t atoi64_result = atoi64_legacy(random_string_1);
292286
const int64_t locale_independent_atoi_result = LocaleIndependentAtoi<int64_t>(random_string_1);
293-
assert(atoi64_result == locale_independent_atoi_result || locale_independent_atoi_result == 0);
287+
assert(atoi64_result == locale_independent_atoi_result);
294288
}
295289
}

‎src/test/getarg_tests.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <util/strencodings.h>
77
#include <util/system.h>
88

9+
#include <limits>
910
#include <string>
1011
#include <utility>
1112
#include <vector>
@@ -144,6 +145,11 @@ BOOST_AUTO_TEST_CASE(intarg)
144145
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 11), 0);
145146
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0);
146147

148+
// Check under-/overflow behavior.
149+
ResetArgs("-foo=-9223372036854775809 -bar=9223372036854775808");
150+
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 0), std::numeric_limits<int64_t>::min());
151+
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 0), std::numeric_limits<int64_t>::max());
152+
147153
ResetArgs("-foo=11 -bar=12");
148154
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 0), 11);
149155
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 12);

‎src/test/util_tests.cpp

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424

2525
#include <array>
2626
#include <optional>
27+
#include <limits>
28+
#include <map>
2729
#include <stdint.h>
2830
#include <string.h>
2931
#include <thread>
@@ -1621,6 +1623,11 @@ BOOST_AUTO_TEST_CASE(test_ToIntegral)
16211623
BOOST_CHECK(!ToIntegral<uint8_t>("256"));
16221624
}
16231625

1626+
int64_t atoi64_legacy(const std::string& str)
1627+
{
1628+
return strtoll(str.c_str(), nullptr, 10);
1629+
}
1630+
16241631
BOOST_AUTO_TEST_CASE(test_LocaleIndependentAtoi)
16251632
{
16261633
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("1234"), 1'234);
@@ -1648,48 +1655,68 @@ BOOST_AUTO_TEST_CASE(test_LocaleIndependentAtoi)
16481655
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>(""), 0);
16491656
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("aap"), 0);
16501657
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("0x1"), 0);
1651-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("-32482348723847471234"), 0);
1652-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("32482348723847471234"), 0);
1658+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("-32482348723847471234"), -2'147'483'647 - 1);
1659+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("32482348723847471234"), 2'147'483'647);
16531660

1654-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("-9223372036854775809"), 0);
1661+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("-9223372036854775809"), -9'223'372'036'854'775'807LL - 1LL);
16551662
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("-9223372036854775808"), -9'223'372'036'854'775'807LL - 1LL);
16561663
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("9223372036854775807"), 9'223'372'036'854'775'807);
1657-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("9223372036854775808"), 0);
1664+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>("9223372036854775808"), 9'223'372'036'854'775'807);
1665+
1666+
std::map<std::string, int64_t> atoi64_test_pairs = {
1667+
{"-9223372036854775809", std::numeric_limits<int64_t>::min()},
1668+
{"-9223372036854775808", -9'223'372'036'854'775'807LL - 1LL},
1669+
{"9223372036854775807", 9'223'372'036'854'775'807},
1670+
{"9223372036854775808", std::numeric_limits<int64_t>::max()},
1671+
{"+-", 0},
1672+
{"0x1", 0},
1673+
{"ox1", 0},
1674+
{"", 0},
1675+
};
1676+
1677+
for (const auto& pair : atoi64_test_pairs) {
1678+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>(pair.first), pair.second);
1679+
}
1680+
1681+
// Ensure legacy compatibility with previous versions of Bitcoin Core's atoi64
1682+
for (const auto& pair : atoi64_test_pairs) {
1683+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int64_t>(pair.first), atoi64_legacy(pair.first));
1684+
}
16581685

16591686
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint64_t>("-1"), 0U);
16601687
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint64_t>("0"), 0U);
16611688
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint64_t>("18446744073709551615"), 18'446'744'073'709'551'615ULL);
1662-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint64_t>("18446744073709551616"), 0U);
1689+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint64_t>("18446744073709551616"), 18'446'744'073'709'551'615ULL);
16631690

1664-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("-2147483649"), 0);
1691+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("-2147483649"), -2'147'483'648LL);
16651692
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("-2147483648"), -2'147'483'648LL);
16661693
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("2147483647"), 2'147'483'647);
1667-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("2147483648"), 0);
1694+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int32_t>("2147483648"), 2'147'483'647);
16681695

16691696
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint32_t>("-1"), 0U);
16701697
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint32_t>("0"), 0U);
16711698
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint32_t>("4294967295"), 4'294'967'295U);
1672-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint32_t>("4294967296"), 0U);
1699+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint32_t>("4294967296"), 4'294'967'295U);
16731700

1674-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("-32769"), 0);
1701+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("-32769"), -32'768);
16751702
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("-32768"), -32'768);
16761703
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("32767"), 32'767);
1677-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("32768"), 0);
1704+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int16_t>("32768"), 32'767);
16781705

16791706
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint16_t>("-1"), 0U);
16801707
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint16_t>("0"), 0U);
16811708
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint16_t>("65535"), 65'535U);
1682-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint16_t>("65536"), 0U);
1709+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint16_t>("65536"), 65'535U);
16831710

1684-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("-129"), 0);
1711+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("-129"), -128);
16851712
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("-128"), -128);
16861713
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("127"), 127);
1687-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("128"), 0);
1714+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<int8_t>("128"), 127);
16881715

16891716
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint8_t>("-1"), 0U);
16901717
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint8_t>("0"), 0U);
16911718
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint8_t>("255"), 255U);
1692-
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint8_t>("256"), 0U);
1719+
BOOST_CHECK_EQUAL(LocaleIndependentAtoi<uint8_t>("256"), 255U);
16931720
}
16941721

16951722
BOOST_AUTO_TEST_CASE(test_ParseInt64)

‎src/util/strencodings.h

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include <charconv>
1717
#include <cstdint>
1818
#include <iterator>
19+
#include <limits>
1920
#include <optional>
2021
#include <string>
2122
#include <vector>
@@ -93,8 +94,12 @@ void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut);
9394
// New code should use ToIntegral or the ParseInt* functions
9495
// which provide parse error feedback.
9596
//
96-
// The goal of LocaleIndependentAtoi is to replicate the exact defined behaviour
97-
// of atoi and atoi64 as they behave under the "C" locale.
97+
// The goal of LocaleIndependentAtoi is to replicate the defined behaviour of
98+
// std::atoi as it behaves under the "C" locale, and remove some undefined
99+
// behavior. If the parsed value is bigger than the integer type's maximum
100+
// value, or smaller than the integer type's minimum value, std::atoi has
101+
// undefined behavior, while this function returns the maximum or minimum
102+
// values, respectively.
98103
template <typename T>
99104
T LocaleIndependentAtoi(const std::string& str)
100105
{
@@ -109,7 +114,15 @@ T LocaleIndependentAtoi(const std::string& str)
109114
s = s.substr(1);
110115
}
111116
auto [_, error_condition] = std::from_chars(s.data(), s.data() + s.size(), result);
112-
if (error_condition != std::errc{}) {
117+
if (error_condition == std::errc::result_out_of_range) {
118+
if (s.length() >= 1 && s[0] == '-') {
119+
// Saturate underflow, per strtoll's behavior.
120+
return std::numeric_limits<T>::min();
121+
} else {
122+
// Saturate overflow, per strtoll's behavior.
123+
return std::numeric_limits<T>::max();
124+
}
125+
} else if (error_condition != std::errc{}) {
113126
return 0;
114127
}
115128
return result;

‎test/lint/lint-locale-dependence.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ KNOWN_VIOLATIONS=(
4343
"src/test/dbwrapper_tests.cpp:.*snprintf"
4444
"src/test/fuzz/locale.cpp"
4545
"src/test/fuzz/string.cpp"
46+
"src/test/util_tests.cpp"
4647
)
4748

4849
REGEXP_IGNORE_EXTERNAL_DEPENDENCIES="^src/(crypto/ctaes/|leveldb/|secp256k1/|minisketch/|tinyformat.h|univalue/)"

0 commit comments

Comments
 (0)