diff --git a/lib/token.cpp b/lib/token.cpp index eac69decee7..9649131cafc 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1929,7 +1929,7 @@ const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Sett if (!mImpl->mValues) return nullptr; return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) { - return !v.isImpossible() && v.isIntValue() && v.intvalue <= val; + return !v.isImpossible() && v.isIntValue() && v.intvalue <= val && v.bound != ValueFlow::Value::Bound::Lower; }); } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index baa279803cf..6aa181b830a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1087,6 +1087,24 @@ static void valueFlowImpossibleValues(TokenList& tokenList, const Settings& sett upper.bound = ValueFlow::Value::Bound::Lower; upper.setImpossible(); setTokenValue(tok, std::move(upper), settings); + } else if (tok->isCast() && tok->valueType() && tok->valueType()->isIntegral() && !tok->valueType()->pointer) { + MathLib::bigint minValue; + MathLib::bigint maxValue; + if (!ValueFlow::getMinMaxValues(tok->valueType(), settings.platform, minValue, maxValue)) + continue; + + if (minValue > std::numeric_limits::min()) { + ValueFlow::Value lower{minValue - 1}; + lower.bound = ValueFlow::Value::Bound::Upper; + lower.setImpossible(); + setTokenValue(tok, std::move(lower), settings); + } + if (maxValue < std::numeric_limits::max()) { + ValueFlow::Value upper{maxValue + 1}; + upper.bound = ValueFlow::Value::Bound::Lower; + upper.setImpossible(); + setTokenValue(tok, std::move(upper), settings); + } } else if (astIsUnsigned(tok) && !astIsPointer(tok)) { std::vector minvalue = minUnsignedValue(tok); if (minvalue.empty()) @@ -5138,6 +5156,26 @@ static bool isIntegralOrPointer(const Token* tok) return false; } +/** + * @brief Check if the token is an arithmetic operation whose result type is + * an unsigned integer, i.e. arithmetic that may wrap around. + * + * Impossible bounds cannot be propagated through such arithmetic because + * wrap-around invalidates the bounds. + */ +static bool isUnsignedArithmeticResult(const Token* tok) +{ + if (!Token::Match(tok, "+|-|*")) + return false; + const ValueType* vt = tok->valueType(); + return vt && vt->isIntegral() && vt->pointer == 0 && + vt->sign == ValueType::Sign::UNSIGNED && + (vt->type == ValueType::Type::INT || + vt->type == ValueType::Type::LONG || + vt->type == ValueType::Type::LONGLONG || + vt->type == ValueType::Type::UNKNOWN_INT); +} + static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settings) { for (Token* tok = tokenlist.front(); tok; tok = tok->next()) { @@ -5158,8 +5196,31 @@ static void valueFlowInferCondition(TokenList& tokenlist, const Settings& settin } } } else if (isIntegralOrPointer(tok->astOperand1()) && isIntegralOrPointer(tok->astOperand2())) { + std::list lhsValues = tok->astOperand1()->values(); + std::list rhsValues = tok->astOperand2()->values(); + // Impossible bounds cannot be propagated through arithmetic whose + // result type is unsigned because wrap-around invalidates the bound + const bool isUnsignedArith = isUnsignedArithmeticResult(tok); + const auto isImpossibleIntegralBound = [](const ValueFlow::Value& v) { + return v.isIntValue() && v.isImpossible() && v.bound != ValueFlow::Value::Bound::Point; + }; + if (isUnsignedArith) { + lhsValues.remove_if(isImpossibleIntegralBound); + rhsValues.remove_if(isImpossibleIntegralBound); + // When the impossible bounds are removed, a conditionally + // derived value (e.g. an interprocedural or branch-possible + // value with a different path) could collapse the interval to + // a scalar and produce a point value that loses its condition + // and path provenance. Drop those values as well so only + // unconditional knowledge is used to infer a point value. + const auto isConditionalPossibleValue = [](const ValueFlow::Value& v) { + return v.isIntValue() && !v.isKnown() && (v.condition != nullptr || v.path != 0); + }; + lhsValues.remove_if(isConditionalPossibleValue); + rhsValues.remove_if(isConditionalPossibleValue); + } std::vector result = - infer(makeIntegralInferModel(), tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values()); + infer(makeIntegralInferModel(), tok->str(), lhsValues, rhsValues); for (ValueFlow::Value& value : result) { setTokenValue(tok, std::move(value), settings); } diff --git a/lib/vf_common.cpp b/lib/vf_common.cpp index f9eecc20fb9..94120cdf56d 100644 --- a/lib/vf_common.cpp +++ b/lib/vf_common.cpp @@ -46,7 +46,7 @@ namespace ValueFlow if (!vt || !vt->isIntegral() || vt->pointer) return false; - std::uint8_t bits; + std::size_t bits; switch (vt->type) { case ValueType::Type::BOOL: bits = 1; @@ -66,10 +66,16 @@ namespace ValueFlow case ValueType::Type::LONGLONG: bits = platform.long_long_bit; break; + case ValueType::Type::WCHAR_T: + bits = platform.sizeof_wchar_t * platform.char_bit; + break; default: return false; } + if (bits == 0) { + return false; + } if (bits == 1) { minValue = 0; maxValue = 1; @@ -77,18 +83,20 @@ namespace ValueFlow if (vt->sign == ValueType::Sign::UNSIGNED) { minValue = 0; maxValue = (1LL << bits) - 1; - } else { + } else if (vt->sign == ValueType::Sign::SIGNED) { minValue = -(1LL << (bits - 1)); maxValue = (1LL << (bits - 1)) - 1; - } + } else + return false; } else if (bits == 64) { if (vt->sign == ValueType::Sign::UNSIGNED) { minValue = 0; - maxValue = LLONG_MAX; // todo max unsigned value - } else { + maxValue = LLONG_MAX; // MathLib::bigint cannot represent ULLONG_MAX; conservative max for unsigned 64-bit + } else if (vt->sign == ValueType::Sign::SIGNED) { minValue = LLONG_MIN; maxValue = LLONG_MAX; - } + } else + return false; } else { return false; } diff --git a/lib/vf_settokenvalue.cpp b/lib/vf_settokenvalue.cpp index 1d6179fb9af..e1f402de74c 100644 --- a/lib/vf_settokenvalue.cpp +++ b/lib/vf_settokenvalue.cpp @@ -488,6 +488,18 @@ namespace ValueFlow return; } + // Impossible bounds cannot be propagated through arithmetic whose + // result type is unsigned, because wrap-around invalidates the bound + const ValueType* resultType = parent->valueType(); + const bool wraps = + resultType && resultType->isIntegral() && + resultType->sign == ValueType::Sign::UNSIGNED && resultType->pointer == 0 && + (resultType->type == ValueType::Type::INT || + resultType->type == ValueType::Type::LONG || + resultType->type == ValueType::Type::LONGLONG || + resultType->type == ValueType::Type::UNKNOWN_INT); + const bool skipImpossibleBounds = wraps && Token::Match(parent, "+|-|*"); + for (const Value &value1 : parent->astOperand1()->values()) { if (!isComputableValue(parent, value1)) continue; @@ -500,6 +512,12 @@ namespace ValueFlow continue; if (!isCompatibleValues(value1, value2)) continue; + // Skip impossible bounds on arithmetic with unsigned result type + const bool operandHasImpossibleBound = + (value1.isIntValue() && value1.isImpossible() && value1.bound != Value::Bound::Point) || + (value2.isIntValue() && value2.isImpossible() && value2.bound != Value::Bound::Point); + if (skipImpossibleBounds && operandHasImpossibleBound) + continue; Value result(0); combineValueProperties(value1, value2, result); if (astIsFloat(parent, false)) { diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index f6a2e1a5ec1..bfcad21fa77 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -164,6 +164,7 @@ class TestBufferOverrun : public TestFixture { TEST_CASE(array_index_74); // #11088 TEST_CASE(array_index_75); TEST_CASE(array_index_76); + TEST_CASE(array_index_77); TEST_CASE(array_index_multidim); TEST_CASE(array_index_switch_in_for); TEST_CASE(array_index_for_in_for); // FP: #2634 @@ -2012,6 +2013,34 @@ class TestBufferOverrun : public TestFixture { errout_str()); } + void array_index_77() + { + // The loop index x is at least colsToTranslate, and the cast range of + // (int)cloudDx only provides the trivial floor of int. x - colsToTranslate + // must not be reported as a negative index. + check("static float cloudDx;\n" + "static int g_dst[400];\n" + "static int g_src[400];\n" + "void updateClouds(float elapsedTime) {\n" + " cloudDx += elapsedTime * 5;\n" + " if (cloudDx >= 1.0f) {\n" + " const int colsToTranslate = (int)cloudDx;\n" + " for (int x = colsToTranslate; x < 400; ++x) {\n" + " g_dst[x - colsToTranslate] = g_src[x];\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // A genuinely negative literal index must still be reported + check("static int a[10];\n" + "void f() {\n" + " a[-1] = 0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3:4]: (error) Array 'a[10]' accessed at index -1, which is out of bounds. [negativeIndex]\n", + errout_str()); + } + void array_index_multidim() { check("void f()\n" "{\n" diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 4d989804b36..1cc2df641a3 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -6631,6 +6631,99 @@ class TestCondition : public TestFixture { "[test.cpp:4:13]: (style) Comparing expression of type 'const unsigned int &' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n", errout_str()); + // no false positive when unsigned arithmetic wraps around + check("void f(int x) {\n" + " if ((unsigned int)x - 1u == 0xFFFFFFFFu) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("", errout_str()); + + check("void f(int x) {\n" + " if ((unsigned short)x - 1u == 0xFFFFFFFFu) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("", errout_str()); + + // Signed result type after integer promotion must still warn + check("void f(int x) {\n" + " if ((unsigned short)x + 1 == 0) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:29]: (style) Condition '(unsigned short)x+1==0' is always false [knownConditionTrueFalse]\n", + errout_str()); + + // Direct cast bounds are preserved + check("void f(int x) {\n" + " if ((unsigned int)x > 4294967295ULL) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:25]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n", + errout_str()); + + // Reproduce the original typedef-heavy Simulink-generated pattern from #15000. + check("void f(unsigned int x) {\n" + " unsigned long long tmp = ((unsigned long long)x) + 1ULL;\n" + " if (tmp > 4294967295ULL)\n" + " tmp = 4294967295ULL;\n" + " if ((((long long)((unsigned int)tmp)) - 1LL) < 0LL) {}\n" + " if ((((long long)((unsigned int)tmp)) - 1LL) > 4294967295LL) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:6:50]: (style) Condition '(((long long)((unsigned int)tmp))-1LL)>4294967295LL' is always false [knownConditionTrueFalse]\n", + errout_str()); + + check("void f(unsigned long long tmp) {\n" + " if (tmp > 4294967295ULL)\n" + " tmp = 4294967295ULL;\n" + " if (static_cast(static_cast(tmp)) - 1LL > 4294967295LL) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:4:70]: (style) Condition 'static_cast(static_cast(tmp))-1LL>4294967295LL' is always false [knownConditionTrueFalse]\n", + errout_str()); + + // cast directly around variable: both the range-based and the declared-type + // analysis can prove the condition invariant; diag() must prevent duplicates + check("void f(unsigned char c) {\n" + " if ((unsigned char)c > 255) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:28]: (style) Comparing expression of type 'unsigned char' against value 255. Condition is always false. [compareValueOutOfTypeRangeError]\n", + errout_str()); + + check("void f(unsigned char c) {\n" + " if ((unsigned char)c == 256) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:29]: (style) Comparing expression of type 'unsigned char' against value 256. Condition is always false. [compareValueOutOfTypeRangeError]\n", + errout_str()); + + check("void f(unsigned int u) {\n" + " if ((unsigned int)u > 4294967295ULL) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n", + errout_str()); + + check("void f(unsigned short s) {\n" + " if ((unsigned int)s > 4294967295ULL) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n", + errout_str()); + + // wchar_t range is derived through ValueType::getSizeOf() + check("void f(wchar_t c) {\n" + " if ((wchar_t)c > 0x7fffffff) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:20]: (style) Condition '(wchar_t)c>0x7fffffff' is always false [knownConditionTrueFalse]\n", + errout_str()); + + check("void f(unsigned int x) {\n" + " if (-(signed char)x < -129) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("[test.cpp:2:25]: (style) Condition '-(char)x<-129' is always false [knownConditionTrueFalse]\n", + errout_str()); + + check("void f(int x) {\n" + " if ((x) > 0) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("", errout_str()); + + check("void f(unsigned int x) {\n" + " if ((unsigned int)x > 0) {}\n" + "}\n", settingsUnix64); + ASSERT_EQUALS("", errout_str()); + check("void f() {\n" " long long ll = 1024 * 1024 * 1024;\n" " if (ll * 8 < INT_MAX) {}\n" diff --git a/test/testtype.cpp b/test/testtype.cpp index 1b5b57f41ca..09b3b582fed 100644 --- a/test/testtype.cpp +++ b/test/testtype.cpp @@ -636,7 +636,28 @@ class TestType : public TestFixture { "};\n" "static B<64> b;\n"); ASSERT_EQUALS("", errout_str()); + + // FP shiftTooManyBits: guarded shift where the shift amount is + // conditionally derived from an unsigned subtraction; a possible value + // from an interprocedural caller must not collapse the interval + // inference on the unsigned arithmetic result + { + const Settings settings = settingsBuilder().platform(Platform::Type::Unix64).build(); + check("static unsigned int l[16];\n" + "void scalar_shift(unsigned int shift) {\n" + " unsigned int shiftlimbs = shift >> 5;\n" + " unsigned int shiftlow = shift & 0x1Fu;\n" + " unsigned int shifthigh = 32u - shiftlow;\n" + " unsigned int r = 0u;\n" + " r |= (shift < 448u && shiftlow ? (l[1 + shiftlimbs] << shifthigh) : 0u);\n" + " r |= (shift < 416u && shiftlow ? (l[2 + shiftlimbs] << shifthigh) : 0u);\n" + " (void)r;\n" + "}\n" + "int main(void) { scalar_shift(384u); return 0; }\n", + dinit(CheckOptions, $.settings = &settings)); + ASSERT_EQUALS("", errout_str()); + } } }; -REGISTER_TEST(TestType) +REGISTER_TEST(TestType) \ No newline at end of file diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index b192a34eb7f..635ee3c0630 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -9613,6 +9613,49 @@ class TestValueFlow : public TestFixture { "}\n"; ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, "a", -1)); ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, -1)); + + const Settings settingsUnix64 = settingsBuilder().platform(Platform::Type::Unix64).build(); + code = "void f(unsigned long long x) {\n" + " return (unsigned int)x;\n" + "}\n"; + SimpleTokenizer tokenizer(settingsUnix64, *this); + ASSERT(tokenizer.tokenize(code)); + const Token* returnTok = Token::findmatch(tokenizer.tokens(), "return ("); + ASSERT(returnTok && returnTok->next()); + const std::list& castValues = returnTok->next()->values(); + ASSERT(std::any_of(castValues.cbegin(), castValues.cend(), [](const ValueFlow::Value& value) { + return value.isImpossible() && value.intvalue == -1; + })); + ASSERT(std::any_of(castValues.cbegin(), castValues.cend(), [](const ValueFlow::Value& value) { + return value.isImpossible() && value.intvalue == 4294967296; + })); + + // Impossible bounds on a cast must not be propagated through + // arithmetic with unsigned result type, since wrap-around invalidates the bound + code = "void f(int x) {\n" + " return (unsigned int)x - 1u;\n" + "}\n"; + SimpleTokenizer tokenizer2(settingsUnix64, *this); + ASSERT(tokenizer2.tokenize(code)); + const Token* minusTok = Token::findsimplematch(tokenizer2.tokens(), "-"); + ASSERT(minusTok); + for (const ValueFlow::Value& value : minusTok->values()) { + ASSERT(!(value.isImpossible() && value.isIntValue() && + value.bound != ValueFlow::Value::Bound::Point && value.intvalue == 4294967295)); + } + + // Known values are still propagated through unsigned arithmetic + code = "void f(int x) {\n" + " unsigned int y = 5u;\n" + " return y - 1u;\n" + "}\n"; + SimpleTokenizer tokenizer3(settingsUnix64, *this); + ASSERT(tokenizer3.tokenize(code)); + const Token* minusTok3 = Token::findsimplematch(tokenizer3.tokens(), "-"); + ASSERT(minusTok3); + ASSERT(std::any_of(minusTok3->values().cbegin(), minusTok3->values().cend(), [](const ValueFlow::Value& value) { + return value.isKnown() && value.isIntValue() && value.intvalue == 4; + })); } void valueFlowImpossibleIncDec()