Skip to content

Commit b5c9bb5

Browse files
jamesobryanofsky
jamesob
and
ryanofsky
committed
util: Restore GetIntArg saturating behavior
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. Co-authored-by: Ryan Ofsky <[email protected]>
1 parent c561f2f commit b5c9bb5

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)