Skip to content

Commit 89e7250

Browse files
jfdevergeJean-François DEVERGE
jfdeverge
authored and
Jean-François DEVERGE
committed
Fix #15000: Propagate integral cast ranges to condition analysis
Impossible bounds on integral casts must not be propagated through arithmetic with unsigned result type, because wrap-around invalidates the bounds. '(uint32_t)x - 1u == 0xFFFFFFFFu' must not be reported as an always-false condition, since 0u - 1u wraps to 4294967295. - Filter impossible integral bounds from the operand values before interval inference in valueFlowInferCondition() when the arithmetic result type is an unsigned integer (+, -, *). - Skip impossible-bounded operand pairs in the binary calculation branch of setTokenValue() under the same result-type predicate. - Known and possible values, impossible point values, comparisons, pointer arithmetic and arithmetic with signed result type are preserved.
1 parent 746b3a7 commit 89e7250

8 files changed

Lines changed: 282 additions & 9 deletions

File tree

‎lib/token.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1929,7 +1929,7 @@ const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Sett
19291929
if (!mImpl->mValues)
19301930
return nullptr;
19311931
return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
1932-
return !v.isImpossible() && v.isIntValue() && v.intvalue <= val;
1932+
return !v.isImpossible() && v.isIntValue() && v.intvalue <= val && v.bound != ValueFlow::Value::Bound::Lower;
19331933
});
19341934
}
19351935

‎lib/valueflow.cpp‎

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1087,6 +1087,24 @@ static void valueFlowImpossibleValues(TokenList& tokenList, const Settings& sett
10871087
upper.bound = ValueFlow::Value::Bound::Lower;
10881088
upper.setImpossible();
10891089
setTokenValue(tok, std::move(upper), settings);
1090+
} else if (tok->isCast() && tok->valueType() && tok->valueType()->isIntegral() && !tok->valueType()->pointer) {
1091+
MathLib::bigint minValue;
1092+
MathLib::bigint maxValue;
1093+
if (!ValueFlow::getMinMaxValues(tok->valueType(), settings.platform, minValue, maxValue))
1094+
continue;
1095+
1096+
if (minValue > std::numeric_limits<MathLib::bigint>::min()) {
1097+
ValueFlow::Value lower{minValue - 1};
1098+
lower.bound = ValueFlow::Value::Bound::Upper;
1099+
lower.setImpossible();
1100+
setTokenValue(tok, std::move(lower), settings);
1101+
}
1102+
if (maxValue < std::numeric_limits<MathLib::bigint>::max()) {
1103+
ValueFlow::Value upper{maxValue + 1};
1104+
upper.bound = ValueFlow::Value::Bound::Lower;
1105+
upper.setImpossible();
1106+
setTokenValue(tok, std::move(upper), settings);
1107+
}
10901108
} else if (astIsUnsigned(tok) && !astIsPointer(tok)) {
10911109
std::vector<MathLib::bigint> minvalue = minUnsignedValue(tok);
10921110
if (minvalue.empty())
@@ -5138,6 +5156,26 @@ static bool isIntegralOrPointer(const Token* tok)
51385156
return false;
51395157
}
51405158

5159+
/**
5160+
* @brief Check if the token is an arithmetic operation whose result type is
5161+
* an unsigned integer, i.e. arithmetic that may wrap around.
5162+
*
5163+
* Impossible bounds cannot be propagated through such arithmetic because
5164+
* wrap-around invalidates the bounds.
5165+
*/
5166+
static bool isUnsignedArithmeticResult(const Token* tok)
5167+
{
5168+
if (!Token::Match(tok, "+|-|*"))
5169+
return false;
5170+
const ValueType* vt = tok->valueType();
5171+
return vt && vt->isIntegral() && vt->pointer == 0 &&
5172+
vt->sign == ValueType::Sign::UNSIGNED &&
5173+
(vt->type == ValueType::Type::INT ||
5174+
vt->type == ValueType::Type::LONG ||
5175+
vt->type == ValueType::Type::LONGLONG ||
5176+
vt->type == ValueType::Type::UNKNOWN_INT);
5177+
}
5178+
51415179
static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settings)
51425180
{
51435181
for (Token* tok = tokenlist.front(); tok; tok = tok->next()) {
@@ -5158,8 +5196,31 @@ static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settin
51585196
}
51595197
}
51605198
} else if (isIntegralOrPointer(tok->astOperand1()) && isIntegralOrPointer(tok->astOperand2())) {
5199+
std::list<ValueFlow::Value> lhsValues = tok->astOperand1()->values();
5200+
std::list<ValueFlow::Value> rhsValues = tok->astOperand2()->values();
5201+
// Impossible bounds cannot be propagated through arithmetic whose
5202+
// result type is unsigned because wrap-around invalidates the bound
5203+
const bool isUnsignedArith = isUnsignedArithmeticResult(tok);
5204+
const auto isImpossibleIntegralBound = [](const ValueFlow::Value& v) {
5205+
return v.isIntValue() && v.isImpossible() && v.bound != ValueFlow::Value::Bound::Point;
5206+
};
5207+
if (isUnsignedArith) {
5208+
lhsValues.remove_if(isImpossibleIntegralBound);
5209+
rhsValues.remove_if(isImpossibleIntegralBound);
5210+
// When the impossible bounds are removed, a conditionally
5211+
// derived value (e.g. an interprocedural or branch-possible
5212+
// value with a different path) could collapse the interval to
5213+
// a scalar and produce a point value that loses its condition
5214+
// and path provenance. Drop those values as well so only
5215+
// unconditional knowledge is used to infer a point value.
5216+
const auto isConditionalPossibleValue = [](const ValueFlow::Value& v) {
5217+
return v.isIntValue() && !v.isKnown() && (v.condition != nullptr || v.path != 0);
5218+
};
5219+
lhsValues.remove_if(isConditionalPossibleValue);
5220+
rhsValues.remove_if(isConditionalPossibleValue);
5221+
}
51615222
std::vector<ValueFlow::Value> result =
5162-
infer(makeIntegralInferModel(), tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values());
5223+
infer(makeIntegralInferModel(), tok->str(), lhsValues, rhsValues);
51635224
for (ValueFlow::Value& value : result) {
51645225
setTokenValue(tok, std::move(value), settings);
51655226
}

‎lib/vf_common.cpp‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ namespace ValueFlow
4646
if (!vt || !vt->isIntegral() || vt->pointer)
4747
return false;
4848

49-
std::uint8_t bits;
49+
std::size_t bits;
5050
switch (vt->type) {
5151
case ValueType::Type::BOOL:
5252
bits = 1;
@@ -66,29 +66,37 @@ namespace ValueFlow
6666
case ValueType::Type::LONGLONG:
6767
bits = platform.long_long_bit;
6868
break;
69+
case ValueType::Type::WCHAR_T:
70+
bits = platform.sizeof_wchar_t * platform.char_bit;
71+
break;
6972
default:
7073
return false;
7174
}
7275

76+
if (bits == 0) {
77+
return false;
78+
}
7379
if (bits == 1) {
7480
minValue = 0;
7581
maxValue = 1;
7682
} else if (bits < 62) {
7783
if (vt->sign == ValueType::Sign::UNSIGNED) {
7884
minValue = 0;
7985
maxValue = (1LL << bits) - 1;
80-
} else {
86+
} else if (vt->sign == ValueType::Sign::SIGNED) {
8187
minValue = -(1LL << (bits - 1));
8288
maxValue = (1LL << (bits - 1)) - 1;
83-
}
89+
} else
90+
return false;
8491
} else if (bits == 64) {
8592
if (vt->sign == ValueType::Sign::UNSIGNED) {
8693
minValue = 0;
87-
maxValue = LLONG_MAX; // todo max unsigned value
88-
} else {
94+
maxValue = LLONG_MAX; // MathLib::bigint cannot represent ULLONG_MAX; conservative max for unsigned 64-bit
95+
} else if (vt->sign == ValueType::Sign::SIGNED) {
8996
minValue = LLONG_MIN;
9097
maxValue = LLONG_MAX;
91-
}
98+
} else
99+
return false;
92100
} else {
93101
return false;
94102
}

‎lib/vf_settokenvalue.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,18 @@ namespace ValueFlow
488488
return;
489489
}
490490

491+
// Impossible bounds cannot be propagated through arithmetic whose
492+
// result type is unsigned, because wrap-around invalidates the bound
493+
const ValueType* resultType = parent->valueType();
494+
const bool wraps =
495+
resultType && resultType->isIntegral() &&
496+
resultType->sign == ValueType::Sign::UNSIGNED && resultType->pointer == 0 &&
497+
(resultType->type == ValueType::Type::INT ||
498+
resultType->type == ValueType::Type::LONG ||
499+
resultType->type == ValueType::Type::LONGLONG ||
500+
resultType->type == ValueType::Type::UNKNOWN_INT);
501+
const bool skipImpossibleBounds = wraps && Token::Match(parent, "+|-|*");
502+
491503
for (const Value &value1 : parent->astOperand1()->values()) {
492504
if (!isComputableValue(parent, value1))
493505
continue;
@@ -500,6 +512,12 @@ namespace ValueFlow
500512
continue;
501513
if (!isCompatibleValues(value1, value2))
502514
continue;
515+
// Skip impossible bounds on arithmetic with unsigned result type
516+
const bool operandHasImpossibleBound =
517+
(value1.isIntValue() && value1.isImpossible() && value1.bound != Value::Bound::Point) ||
518+
(value2.isIntValue() && value2.isImpossible() && value2.bound != Value::Bound::Point);
519+
if (skipImpossibleBounds && operandHasImpossibleBound)
520+
continue;
503521
Value result(0);
504522
combineValueProperties(value1, value2, result);
505523
if (astIsFloat(parent, false)) {

‎test/testbufferoverrun.cpp‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ class TestBufferOverrun : public TestFixture {
164164
TEST_CASE(array_index_74); // #11088
165165
TEST_CASE(array_index_75);
166166
TEST_CASE(array_index_76);
167+
TEST_CASE(array_index_77);
167168
TEST_CASE(array_index_multidim);
168169
TEST_CASE(array_index_switch_in_for);
169170
TEST_CASE(array_index_for_in_for); // FP: #2634
@@ -2012,6 +2013,34 @@ class TestBufferOverrun : public TestFixture {
20122013
errout_str());
20132014
}
20142015

2016+
void array_index_77()
2017+
{
2018+
// The loop index x is at least colsToTranslate, and the cast range of
2019+
// (int)cloudDx only provides the trivial floor of int. x - colsToTranslate
2020+
// must not be reported as a negative index.
2021+
check("static float cloudDx;\n"
2022+
"static int g_dst[400];\n"
2023+
"static int g_src[400];\n"
2024+
"void updateClouds(float elapsedTime) {\n"
2025+
" cloudDx += elapsedTime * 5;\n"
2026+
" if (cloudDx >= 1.0f) {\n"
2027+
" const int colsToTranslate = (int)cloudDx;\n"
2028+
" for (int x = colsToTranslate; x < 400; ++x) {\n"
2029+
" g_dst[x - colsToTranslate] = g_src[x];\n"
2030+
" }\n"
2031+
" }\n"
2032+
"}\n");
2033+
ASSERT_EQUALS("", errout_str());
2034+
2035+
// A genuinely negative literal index must still be reported
2036+
check("static int a[10];\n"
2037+
"void f() {\n"
2038+
" a[-1] = 0;\n"
2039+
"}\n");
2040+
ASSERT_EQUALS("[test.cpp:3:4]: (error) Array 'a[10]' accessed at index -1, which is out of bounds. [negativeIndex]\n",
2041+
errout_str());
2042+
}
2043+
20152044
void array_index_multidim() {
20162045
check("void f()\n"
20172046
"{\n"

‎test/testcondition.cpp‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6631,6 +6631,99 @@ class TestCondition : public TestFixture {
66316631
"[test.cpp:4:13]: (style) Comparing expression of type 'const unsigned int &' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
66326632
errout_str());
66336633

6634+
// no false positive when unsigned arithmetic wraps around
6635+
check("void f(int x) {\n"
6636+
" if ((unsigned int)x - 1u == 0xFFFFFFFFu) {}\n"
6637+
"}\n", settingsUnix64);
6638+
ASSERT_EQUALS("", errout_str());
6639+
6640+
check("void f(int x) {\n"
6641+
" if ((unsigned short)x - 1u == 0xFFFFFFFFu) {}\n"
6642+
"}\n", settingsUnix64);
6643+
ASSERT_EQUALS("", errout_str());
6644+
6645+
// Signed result type after integer promotion must still warn
6646+
check("void f(int x) {\n"
6647+
" if ((unsigned short)x + 1 == 0) {}\n"
6648+
"}\n", settingsUnix64);
6649+
ASSERT_EQUALS("[test.cpp:2:29]: (style) Condition '(unsigned short)x+1==0' is always false [knownConditionTrueFalse]\n",
6650+
errout_str());
6651+
6652+
// Direct cast bounds are preserved
6653+
check("void f(int x) {\n"
6654+
" if ((unsigned int)x > 4294967295ULL) {}\n"
6655+
"}\n", settingsUnix64);
6656+
ASSERT_EQUALS("[test.cpp:2:25]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
6657+
errout_str());
6658+
6659+
// Reproduce the original typedef-heavy Simulink-generated pattern from #15000.
6660+
check("void f(unsigned int x) {\n"
6661+
" unsigned long long tmp = ((unsigned long long)x) + 1ULL;\n"
6662+
" if (tmp > 4294967295ULL)\n"
6663+
" tmp = 4294967295ULL;\n"
6664+
" if ((((long long)((unsigned int)tmp)) - 1LL) < 0LL) {}\n"
6665+
" if ((((long long)((unsigned int)tmp)) - 1LL) > 4294967295LL) {}\n"
6666+
"}\n", settingsUnix64);
6667+
ASSERT_EQUALS("[test.cpp:6:50]: (style) Condition '(((long long)((unsigned int)tmp))-1LL)>4294967295LL' is always false [knownConditionTrueFalse]\n",
6668+
errout_str());
6669+
6670+
check("void f(unsigned long long tmp) {\n"
6671+
" if (tmp > 4294967295ULL)\n"
6672+
" tmp = 4294967295ULL;\n"
6673+
" if (static_cast<long long>(static_cast<unsigned int>(tmp)) - 1LL > 4294967295LL) {}\n"
6674+
"}\n", settingsUnix64);
6675+
ASSERT_EQUALS("[test.cpp:4:70]: (style) Condition 'static_cast<long long>(static_cast<unsigned int>(tmp))-1LL>4294967295LL' is always false [knownConditionTrueFalse]\n",
6676+
errout_str());
6677+
6678+
// cast directly around variable: both the range-based and the declared-type
6679+
// analysis can prove the condition invariant; diag() must prevent duplicates
6680+
check("void f(unsigned char c) {\n"
6681+
" if ((unsigned char)c > 255) {}\n"
6682+
"}\n", settingsUnix64);
6683+
ASSERT_EQUALS("[test.cpp:2:28]: (style) Comparing expression of type 'unsigned char' against value 255. Condition is always false. [compareValueOutOfTypeRangeError]\n",
6684+
errout_str());
6685+
6686+
check("void f(unsigned char c) {\n"
6687+
" if ((unsigned char)c == 256) {}\n"
6688+
"}\n", settingsUnix64);
6689+
ASSERT_EQUALS("[test.cpp:2:29]: (style) Comparing expression of type 'unsigned char' against value 256. Condition is always false. [compareValueOutOfTypeRangeError]\n",
6690+
errout_str());
6691+
6692+
check("void f(unsigned int u) {\n"
6693+
" if ((unsigned int)u > 4294967295ULL) {}\n"
6694+
"}\n", settingsUnix64);
6695+
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
6696+
errout_str());
6697+
6698+
check("void f(unsigned short s) {\n"
6699+
" if ((unsigned int)s > 4294967295ULL) {}\n"
6700+
"}\n", settingsUnix64);
6701+
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
6702+
errout_str());
6703+
6704+
// wchar_t range is derived through ValueType::getSizeOf()
6705+
check("void f(wchar_t c) {\n"
6706+
" if ((wchar_t)c > 0x7fffffff) {}\n"
6707+
"}\n", settingsUnix64);
6708+
ASSERT_EQUALS("[test.cpp:2:20]: (style) Condition '(wchar_t)c>0x7fffffff' is always false [knownConditionTrueFalse]\n",
6709+
errout_str());
6710+
6711+
check("void f(unsigned int x) {\n"
6712+
" if (-(signed char)x < -129) {}\n"
6713+
"}\n", settingsUnix64);
6714+
ASSERT_EQUALS("[test.cpp:2:25]: (style) Condition '-(char)x<-129' is always false [knownConditionTrueFalse]\n",
6715+
errout_str());
6716+
6717+
check("void f(int x) {\n"
6718+
" if ((x) > 0) {}\n"
6719+
"}\n", settingsUnix64);
6720+
ASSERT_EQUALS("", errout_str());
6721+
6722+
check("void f(unsigned int x) {\n"
6723+
" if ((unsigned int)x > 0) {}\n"
6724+
"}\n", settingsUnix64);
6725+
ASSERT_EQUALS("", errout_str());
6726+
66346727
check("void f() {\n"
66356728
" long long ll = 1024 * 1024 * 1024;\n"
66366729
" if (ll * 8 < INT_MAX) {}\n"

‎test/testtype.cpp‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,28 @@ class TestType : public TestFixture {
636636
"};\n"
637637
"static B<64> b;\n");
638638
ASSERT_EQUALS("", errout_str());
639+
640+
// FP shiftTooManyBits: guarded shift where the shift amount is
641+
// conditionally derived from an unsigned subtraction; a possible value
642+
// from an interprocedural caller must not collapse the interval
643+
// inference on the unsigned arithmetic result
644+
{
645+
const Settings settings = settingsBuilder().platform(Platform::Type::Unix64).build();
646+
check("static unsigned int l[16];\n"
647+
"void scalar_shift(unsigned int shift) {\n"
648+
" unsigned int shiftlimbs = shift >> 5;\n"
649+
" unsigned int shiftlow = shift & 0x1Fu;\n"
650+
" unsigned int shifthigh = 32u - shiftlow;\n"
651+
" unsigned int r = 0u;\n"
652+
" r |= (shift < 448u && shiftlow ? (l[1 + shiftlimbs] << shifthigh) : 0u);\n"
653+
" r |= (shift < 416u && shiftlow ? (l[2 + shiftlimbs] << shifthigh) : 0u);\n"
654+
" (void)r;\n"
655+
"}\n"
656+
"int main(void) { scalar_shift(384u); return 0; }\n",
657+
dinit(CheckOptions, $.settings = &settings));
658+
ASSERT_EQUALS("", errout_str());
659+
}
639660
}
640661
};
641662

642-
REGISTER_TEST(TestType)
663+
REGISTER_TEST(TestType)

0 commit comments

Comments
 (0)