From 486e440c4a52723aff8129d158406e27724f504b Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 4 Jun 2021 10:17:41 -0500 Subject: [PATCH] Fix 10298: ValueFlow: Wrong known value, 'x == -1' implicit unsigned cast for rhs (#3277) --- lib/checktype.cpp | 5 +++- lib/token.cpp | 44 +++++-------------------------- lib/valueflow.cpp | 60 +++++++++++++++++++++++++++++++++++++++++- lib/valueflow.h | 8 ++++++ test/testcondition.cpp | 6 +++++ test/testtype.cpp | 4 ++- 6 files changed, 86 insertions(+), 41 deletions(-) diff --git a/lib/checktype.cpp b/lib/checktype.cpp index 0cb337861..c4c434c33 100644 --- a/lib/checktype.cpp +++ b/lib/checktype.cpp @@ -241,7 +241,10 @@ void CheckType::checkSignConversion() for (const Token * tok1 : astOperands) { if (!tok1) continue; - const ValueFlow::Value *negativeValue = tok1->getValueLE(-1,mSettings); + const ValueFlow::Value* negativeValue = + ValueFlow::findValue(tok1->values(), mSettings, [&](const ValueFlow::Value& v) { + return !v.isImpossible() && v.isIntValue() && (v.intvalue <= -1 || v.wideintvalue <= -1); + }); if (!negativeValue) continue; if (tok1->valueType() && tok1->valueType()->sign != ValueType::Sign::UNSIGNED) diff --git a/lib/token.cpp b/lib/token.cpp index a1c570f4d..f25e453c8 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1798,50 +1798,18 @@ const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Sett { if (!mImpl->mValues) return nullptr; - const ValueFlow::Value *ret = nullptr; - std::list::const_iterator it; - for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) { - if (it->isImpossible()) - continue; - if (it->isIntValue() && it->intvalue <= val) { - if (!ret || ret->isInconclusive() || (ret->condition && !it->isInconclusive())) - ret = &(*it); - if (!ret->isInconclusive() && !ret->condition) - break; - } - } - if (settings && ret) { - if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive)) - return nullptr; - if (ret->condition && !settings->severity.isEnabled(Severity::warning)) - return nullptr; - } - return ret; + return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) { + return !v.isImpossible() && v.isIntValue() && v.intvalue <= val; + }); } const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Settings *settings) const { if (!mImpl->mValues) return nullptr; - const ValueFlow::Value *ret = nullptr; - std::list::const_iterator it; - for (it = mImpl->mValues->begin(); it != mImpl->mValues->end(); ++it) { - if (it->isImpossible()) - continue; - if (it->isIntValue() && it->intvalue >= val) { - if (!ret || ret->isInconclusive() || (ret->condition && !it->isInconclusive())) - ret = &(*it); - if (!ret->isInconclusive() && !ret->condition) - break; - } - } - if (settings && ret) { - if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive)) - return nullptr; - if (ret->condition && !settings->severity.isEnabled(Severity::warning)) - return nullptr; - } - return ret; + return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) { + return !v.isImpossible() && v.isIntValue() && v.intvalue >= val; + }); } const ValueFlow::Value * Token::getInvalidValue(const Token *ftok, nonneg int argnr, const Settings *settings) const diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d290cbef2..cd16592c3 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -444,13 +444,48 @@ static bool isCompatibleValues(const ValueFlow::Value& value1, const ValueFlow:: return false; } +static ValueFlow::Value truncateImplicitConversion(Token* parent, const ValueFlow::Value& value, const Settings* settings) +{ + if (!value.isIntValue() && !value.isFloatValue()) + return value; + if (!parent) + return value; + if (!parent->isBinaryOp()) + return value; + if (!parent->isConstOp()) + return value; + if (!astIsIntegral(parent->astOperand1(), false)) + return value; + if (!astIsIntegral(parent->astOperand2(), false)) + return value; + const ValueType* vt1 = parent->astOperand1()->valueType(); + const ValueType* vt2 = parent->astOperand2()->valueType(); + // If the sign is the same there is no truncation + if (vt1->sign == vt2->sign) + return value; + size_t n1 = ValueFlow::getSizeOf(*vt1, settings); + size_t n2 = ValueFlow::getSizeOf(*vt2, settings); + ValueType::Sign sign = ValueType::Sign::UNSIGNED; + if (n1 < n2) + sign = vt2->sign; + else if (n1 > n2) + sign = vt1->sign; + ValueFlow::Value v = castValue(value, sign, std::max(n1, n2) * 8); + v.wideintvalue = value.intvalue; + return v; +} + /** set ValueFlow value and perform calculations if possible */ -static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Settings *settings) +static void setTokenValue(Token* tok, ValueFlow::Value value, const Settings* settings) { // Skip setting values that are too big since its ambiguous if (!value.isImpossible() && value.isIntValue() && value.intvalue < 0 && astIsUnsigned(tok) && ValueFlow::getSizeOf(*tok->valueType(), settings) >= sizeof(MathLib::bigint)) return; + + if (!value.isImpossible() && value.isIntValue()) + value = truncateImplicitConversion(tok->astParent(), value, settings); + if (!tok->addValue(value)) return; @@ -6793,6 +6828,7 @@ ValueFlow::Value::Value(const Token* c, long long val) defaultArg(false), indirect(0), path(0), + wideintvalue(0), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) @@ -6934,3 +6970,25 @@ std::string ValueFlow::eitherTheConditionIsRedundant(const Token *condition) } return "Either the condition '" + condition->expressionString() + "' is redundant"; } + +const ValueFlow::Value* ValueFlow::findValue(const std::list& values, + const Settings* settings, + std::function pred) +{ + const ValueFlow::Value* ret = nullptr; + for (const ValueFlow::Value& v : values) { + if (pred(v)) { + if (!ret || ret->isInconclusive() || (ret->condition && !v.isInconclusive())) + ret = &v; + if (!ret->isInconclusive() && !ret->condition) + break; + } + } + if (settings && ret) { + if (ret->isInconclusive() && !settings->certainty.isEnabled(Certainty::inconclusive)) + return nullptr; + if (ret->condition && !settings->severity.isEnabled(Severity::warning)) + return nullptr; + } + return ret; +} diff --git a/lib/valueflow.h b/lib/valueflow.h index f305bf412..c655dcfc9 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -95,6 +95,7 @@ namespace ValueFlow { defaultArg(false), indirect(0), path(0), + wideintvalue(val), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) @@ -319,6 +320,9 @@ namespace ValueFlow { /** Path id */ MathLib::bigint path; + /** int value before implicit truncation */ + long long wideintvalue; + enum class LifetimeKind {Object, SubObject, Lambda, Iterator, Address} lifetimeKind; enum class LifetimeScope { Local, Argument, SubFunction } lifetimeScope; @@ -389,6 +393,10 @@ namespace ValueFlow { std::string eitherTheConditionIsRedundant(const Token *condition); size_t getSizeOf(const ValueType &vt, const Settings *settings); + + const ValueFlow::Value* findValue(const std::list& values, + const Settings* settings, + std::function pred); } struct LifetimeToken { diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 90f661ec1..11d2e292f 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3626,6 +3626,12 @@ private: " if ((100 - x) > 0) {}\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // #10298 + check("void foo(unsigned int x) {\n" + " if (x == -1) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void alwaysTrueInfer() { diff --git a/test/testtype.cpp b/test/testtype.cpp index 9381d37e0..688ac2761 100644 --- a/test/testtype.cpp +++ b/test/testtype.cpp @@ -258,6 +258,8 @@ private: } void signConversion() { + Settings settings; + settings.platform(Settings::Unix64); check("x = -4 * (unsigned)y;"); ASSERT_EQUALS("[test.cpp:1]: (warning) Expression '-4' has a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str()); @@ -267,7 +269,7 @@ private: check("unsigned int dostuff(int x) {\n" // x is signed " if (x==0) {}\n" " return (x-1)*sizeof(int);\n" - "}"); + "}", &settings); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Expression 'x-1' can have a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str()); check("unsigned int f1(signed int x, unsigned int y) {" // x is signed