Skip to content

Commit ae29bb6

Browse files
TayweeTaywee
authored
Merge pull request #160 from metsw24-max/validate-allocation-arithmetic-in-importer-parsing-paths
integer overflow hardening in input-derived arithmetic
2 parents 7e920e3 + 3ee0975 commit ae29bb6

3 files changed

Lines changed: 292 additions & 13 deletions

File tree

‎args.hxx‎

Lines changed: 92 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,76 @@ namespace args
101101
return length;
102102
}
103103

104+
/** Safe addition to prevent integer overflow.
105+
* Returns true if the addition is successful, false if it would overflow.
106+
*/
107+
template<typename T>
108+
bool SafeAdd(T a, T b, T& out)
109+
{
110+
static_assert(std::is_integral<T>::value, "SafeAdd requires integral types.");
111+
if (std::is_unsigned<T>::value)
112+
{
113+
if (b > std::numeric_limits<T>::max() - a)
114+
{
115+
return false;
116+
}
117+
} else
118+
{
119+
if ((b > 0 && a > std::numeric_limits<T>::max() - b) ||
120+
(b < 0 && a < std::numeric_limits<T>::min() - b))
121+
{
122+
return false;
123+
}
124+
}
125+
out = a + b;
126+
return true;
127+
}
128+
129+
/** Safe multiplication to prevent integer overflow.
130+
* Returns true if the multiplication is successful, false if it would overflow.
131+
*/
132+
template<typename T>
133+
bool SafeMultiply(T a, T b, T& out)
134+
{
135+
static_assert(std::is_integral<T>::value, "SafeMultiply requires integral types.");
136+
137+
if (a == 0 || b == 0)
138+
{
139+
out = 0;
140+
return true;
141+
}
142+
143+
if (std::is_unsigned<T>::value)
144+
{
145+
if (b > std::numeric_limits<T>::max() / a)
146+
{
147+
return false; // Overflow would occur
148+
}
149+
}
150+
else
151+
{
152+
if (a == -1 && b == std::numeric_limits<T>::min())
153+
{
154+
return false;
155+
}
156+
if (b == -1 && a == std::numeric_limits<T>::min())
157+
{
158+
return false;
159+
}
160+
161+
if ((a > 0 && b > 0 && a > std::numeric_limits<T>::max() / b) ||
162+
(a > 0 && b < 0 && b < std::numeric_limits<T>::min() / a) ||
163+
(a < 0 && b > 0 && a < std::numeric_limits<T>::min() / b) ||
164+
(a < 0 && b < 0 && a < std::numeric_limits<T>::max() / b))
165+
{
166+
return false;
167+
}
168+
}
169+
170+
out = a * b;
171+
return true;
172+
}
173+
104174
/** (INTERNAL) Wrap a vector of words into a vector of lines
105175
*
106176
* Empty words are skipped. Word "\n" forces wrapping.
@@ -152,17 +222,20 @@ namespace args
152222

153223
auto itemsize = Glyphs(*it);
154224

155-
// Security fix: Safe comparison that avoids undefined behavior from unsigned integer overflow
156-
// The original expression (line.length() + 1 + itemsize) > currentwidth performs addition
157-
// of three size_t values which could overflow. This refactored version prevents that.
225+
// Refactored to prevent integer overflow
158226
bool needsWrap = false;
159227
if (itemsize >= currentwidth)
160228
{
161229
needsWrap = true;
162230
}
163-
else if (line.length() + 1 > currentwidth - itemsize)
231+
else
164232
{
165-
needsWrap = true;
233+
size_t remainingWidth = (currentwidth > itemsize) ? (currentwidth - itemsize) : 0;
234+
size_t nextLength = 0;
235+
if (!SafeAdd<std::string::size_type>(line.length(), static_cast<std::string::size_type>(1), nextLength) || nextLength > remainingWidth)
236+
{
237+
needsWrap = true;
238+
}
166239
}
167240

168241
if (needsWrap)
@@ -2348,8 +2421,8 @@ namespace args
23482421
bool allowSeparateShortValue = true;
23492422
bool allowSeparateLongValue = true;
23502423

2351-
CompletionFlag *completion = nullptr;
23522424
bool readCompletion = false;
2425+
CompletionFlag *completion = nullptr;
23532426

23542427
protected:
23552428
enum class OptionType
@@ -2907,12 +2980,17 @@ namespace args
29072980
{
29082981
size_t prev_idx = idx - 1; // Safe since we checked idx > 0
29092982
curArgs[prev_idx] += "=";
2910-
if (idx + 1 < curArgs.size())
2983+
size_t next_idx = 0;
2984+
if (SafeAdd<size_t>(idx, static_cast<size_t>(1), next_idx) && next_idx < curArgs.size())
29112985
{
2912-
curArgs[prev_idx] += curArgs[idx + 1];
2986+
curArgs[prev_idx] += curArgs[next_idx];
29132987
// Erase the '=' token and the following value token.
2914-
curArgs.erase(curArgs.begin() + idx,
2915-
curArgs.begin() + idx + 2);
2988+
size_t erase_end = 0;
2989+
if (SafeAdd<size_t>(next_idx, static_cast<size_t>(1), erase_end))
2990+
{
2991+
curArgs.erase(curArgs.begin() + idx,
2992+
curArgs.begin() + erase_end);
2993+
}
29162994
} else
29172995
{
29182996
// Safe erase of single '=' token at the end
@@ -3024,7 +3102,7 @@ namespace args
30243102
} else
30253103
{
30263104
this->longseparator = longseparator_;
3027-
this->helpParams.longSeparator = allowJoinedLongValue ? longseparator_ : " ";
3105+
this->helpParams.longSeparator = allowJoinedLongValue ? longseparator : " ";
30283106
}
30293107
}
30303108

@@ -4245,7 +4323,8 @@ namespace args
42454323
typedef std::reverse_iterator<iterator> reverse_iterator;
42464324
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
42474325

4248-
MapFlagList(Group &group_, const std::string &name_, const std::string &help_, Matcher &&matcher_, const Map<K, T> &map_, const Container &defaultValues_ = Container()): ValueFlagBase(name_, help_, std::move(matcher_)), map(map_), values(defaultValues_), defaultValues(defaultValues_)
4326+
MapFlagList(Group &group_, const std::string &name_, const std::string &help_, Matcher &&matcher_, const Map<K, T> &map_, const Container &defaultValues_ = Container(), Options options_ = {}):
4327+
ValueFlagBase(name_, help_, std::move(matcher_), options_), map(map_), values(defaultValues_), defaultValues(defaultValues_)
42494328
{
42504329
group_.Add(*this);
42514330
}
@@ -4883,4 +4962,4 @@ namespace args
48834962

48844963
#pragma pop_macro("min")
48854964
#pragma pop_macro("max")
4886-
#endif
4965+
#endif

‎meson.build‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ test_names = [
7777
'required_flags',
7878
'simple_commands',
7979
'single_flag_repeat',
80+
'safe_arithmetic',
8081
'subparser_commands',
8182
'subparser_commands_kickout',
8283
'subparser_group_validation',

‎test/safe_arithmetic.cxx‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
#include "test_common.hxx"
2+
3+
#include <args.hxx>
4+
5+
#include "test_helpers.hxx"
6+
7+
#include <iostream>
8+
#include <limits>
9+
10+
/** Test suite for checked arithmetic functions
11+
*
12+
* This test suite validates the SafeAdd and SafeMultiply helper functions
13+
* that prevent integer overflow in width and allocation arithmetic.
14+
*/
15+
16+
// Test SafeAdd with size_t (the primary type used in Wrap)
17+
void TestSafeAddSizeT()
18+
{
19+
size_t result;
20+
21+
// Test normal addition
22+
test::require(args::SafeAdd<size_t>(5, 10, result));
23+
test::require(result == 15);
24+
std::cout << "PASS: SafeAdd normal case (5 + 10 = 15)" << std::endl;
25+
26+
// Test addition at limit boundary
27+
size_t max_val = std::numeric_limits<size_t>::max();
28+
test::require_false(args::SafeAdd<size_t>(max_val, 1, result));
29+
std::cout << "PASS: SafeAdd overflow detection (SIZE_MAX + 1)" << std::endl;
30+
31+
// Test addition near limit
32+
test::require_false(args::SafeAdd<size_t>(max_val - 5, 10, result));
33+
std::cout << "PASS: SafeAdd near-overflow detection (SIZE_MAX - 5 + 10)" << std::endl;
34+
35+
// Test addition with zero
36+
test::require(args::SafeAdd<size_t>(0, 100, result));
37+
test::require(result == 100);
38+
std::cout << "PASS: SafeAdd with zero (0 + 100 = 100)" << std::endl;
39+
40+
// Test addition at exact boundary
41+
test::require(args::SafeAdd<size_t>(max_val - 5, 5, result));
42+
test::require(result == max_val);
43+
std::cout << "PASS: SafeAdd exact boundary (SIZE_MAX - 5 + 5 = SIZE_MAX)" << std::endl;
44+
}
45+
46+
// Test SafeMultiply with size_t
47+
void TestSafeMultiplySizeT()
48+
{
49+
size_t result;
50+
51+
// Test normal multiplication
52+
test::require(args::SafeMultiply<size_t>(5, 10, result));
53+
test::require(result == 50);
54+
std::cout << "PASS: SafeMultiply normal case (5 * 10 = 50)" << std::endl;
55+
56+
// Test multiplication that would overflow
57+
size_t max_val = std::numeric_limits<size_t>::max();
58+
test::require_false(args::SafeMultiply<size_t>(max_val, 2, result));
59+
std::cout << "PASS: SafeMultiply overflow detection (SIZE_MAX * 2)" << std::endl;
60+
61+
// Test multiplication with zero
62+
test::require(args::SafeMultiply<size_t>(0, 1000000, result));
63+
test::require(result == 0);
64+
std::cout << "PASS: SafeMultiply with zero (0 * 1000000 = 0)" << std::endl;
65+
66+
// Test large multiplications that overflow (for 64-bit systems)
67+
// Use values that will definitely overflow
68+
size_t large_val = 10000000000UL; // 10 billion
69+
test::require_false(args::SafeMultiply<size_t>(large_val, large_val, result));
70+
std::cout << "PASS: SafeMultiply large value overflow (10B * 10B)" << std::endl;
71+
72+
// Test multiplication that doesn't overflow
73+
size_t safe_val = 1000000UL; // 1 million
74+
test::require(args::SafeMultiply<size_t>(safe_val, safe_val, result));
75+
test::require(result == safe_val * safe_val);
76+
std::cout << "PASS: SafeMultiply safe value success (1M * 1M)" << std::endl;
77+
}
78+
79+
// Test SafeAdd with int
80+
void TestSafeAddInt()
81+
{
82+
int result;
83+
84+
// Test normal addition
85+
test::require(args::SafeAdd<int>(100, 200, result));
86+
test::require(result == 300);
87+
std::cout << "PASS: SafeAdd int normal case (100 + 200 = 300)" << std::endl;
88+
89+
// Test negative number handling
90+
test::require(args::SafeAdd<int>(-100, 50, result));
91+
test::require(result == -50);
92+
std::cout << "PASS: SafeAdd int with negative (-100 + 50 = -50)" << std::endl;
93+
94+
// Test negative operand underflow detection
95+
test::require_false(args::SafeAdd<int>(std::numeric_limits<int>::min(), -1, result));
96+
std::cout << "PASS: SafeAdd int underflow detection (INT_MIN + -1)" << std::endl;
97+
98+
// Test int overflow
99+
int max_int = std::numeric_limits<int>::max();
100+
test::require_false(args::SafeAdd<int>(max_int, 1, result));
101+
std::cout << "PASS: SafeAdd int overflow detection (INT_MAX + 1)" << std::endl;
102+
}
103+
104+
// Test SafeMultiply with int
105+
void TestSafeMultiplyInt()
106+
{
107+
int result;
108+
109+
// Test normal multiplication
110+
test::require(args::SafeMultiply<int>(10, 20, result));
111+
test::require(result == 200);
112+
std::cout << "PASS: SafeMultiply int normal case (10 * 20 = 200)" << std::endl;
113+
114+
// Test multiplication overflow
115+
int max_int = std::numeric_limits<int>::max();
116+
test::require_false(args::SafeMultiply<int>(max_int, 2, result));
117+
std::cout << "PASS: SafeMultiply int overflow detection (INT_MAX * 2)" << std::endl;
118+
119+
// Test negative multiplication
120+
test::require(args::SafeMultiply<int>(-10, 20, result));
121+
test::require(result == -200);
122+
std::cout << "PASS: SafeMultiply int negative (-10 * 20 = -200)" << std::endl;
123+
}
124+
125+
// Test Wrap function with boundary conditions
126+
void TestWrapBoundaryConditions()
127+
{
128+
// Test with very large width that could cause overflow
129+
std::vector<std::string> words = {"hello", "world", "test"};
130+
131+
// This should not cause overflow or crash
132+
auto result = args::Wrap(words.begin(), words.end(), std::numeric_limits<size_t>::max());
133+
test::require(!result.empty());
134+
std::cout << "PASS: Wrap with SIZE_MAX width" << std::endl;
135+
136+
// Test with width of 1 (minimal width)
137+
result = args::Wrap(words.begin(), words.end(), 1);
138+
test::require(!result.empty());
139+
std::cout << "PASS: Wrap with width = 1" << std::endl;
140+
141+
// Test with width of 0 (edge case)
142+
result = args::Wrap(words.begin(), words.end(), 0);
143+
test::require(!result.empty());
144+
std::cout << "PASS: Wrap with width = 0" << std::endl;
145+
146+
// Test with empty words vector
147+
std::vector<std::string> empty_words;
148+
result = args::Wrap(empty_words.begin(), empty_words.end(), 80);
149+
test::require(result.empty());
150+
std::cout << "PASS: Wrap with empty words vector" << std::endl;
151+
152+
// Test with newline separator
153+
std::vector<std::string> words_with_newline = {"hello", "\n", "world"};
154+
result = args::Wrap(words_with_newline.begin(), words_with_newline.end(), 80);
155+
test::require(result.size() >= 2);
156+
std::cout << "PASS: Wrap with newline separator" << std::endl;
157+
}
158+
159+
// Test Wrap function with large string input
160+
void TestWrapLargeStringInput()
161+
{
162+
// Test with very long string that could trigger width calculation issues
163+
std::string long_string(1000, 'a');
164+
165+
auto result = args::Wrap(long_string, 80);
166+
test::require(!result.empty());
167+
std::cout << "PASS: Wrap long string (1000 chars) with width 80" << std::endl;
168+
169+
// Test with extremely large width
170+
result = args::Wrap(long_string, std::numeric_limits<size_t>::max());
171+
test::require(result.size() >= 1);
172+
std::cout << "PASS: Wrap long string with SIZE_MAX width" << std::endl;
173+
174+
// Test with width of 1
175+
result = args::Wrap(long_string, 1);
176+
test::require(!result.empty());
177+
std::cout << "PASS: Wrap long string with width 1" << std::endl;
178+
}
179+
180+
// Main test runner
181+
int main()
182+
{
183+
std::cout << "Starting Checked Arithmetic Test Suite\n" << std::endl;
184+
185+
std::cout << "=== SafeAdd Tests ===" << std::endl;
186+
TestSafeAddSizeT();
187+
TestSafeAddInt();
188+
189+
std::cout << "\n=== SafeMultiply Tests ===" << std::endl;
190+
TestSafeMultiplySizeT();
191+
TestSafeMultiplyInt();
192+
193+
std::cout << "\n=== Wrap Function Boundary Tests ===" << std::endl;
194+
TestWrapBoundaryConditions();
195+
TestWrapLargeStringInput();
196+
197+
std::cout << "\n=== All Tests Passed ===" << std::endl;
198+
return 0;
199+
}

0 commit comments

Comments
 (0)