From 035c70c441e795451060dab743676cbd8773b1cb Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 7 Nov 2021 11:19:56 -0600 Subject: [PATCH] Fix 10578: Value not impossible after check (#3549) --- lib/astutils.cpp | 11 +++++++++++ lib/astutils.h | 1 + lib/checkio.cpp | 22 +++++++++++----------- lib/reverseanalyzer.cpp | 9 +++++++++ lib/symboldatabase.cpp | 2 ++ lib/valueflow.cpp | 11 ++++++++++- test/testvalueflow.cpp | 23 ++++++++++++++++++++++- 7 files changed, 66 insertions(+), 13 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 3b71f9f7f..2bf68abcc 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -215,6 +215,14 @@ bool astIsGenericChar(const Token* tok) return !astIsPointer(tok) && tok && tok->valueType() && (tok->valueType()->type == ValueType::Type::CHAR || tok->valueType()->type == ValueType::Type::WCHAR_T); } +bool astIsPrimitive(const Token* tok) +{ + const ValueType* vt = tok ? tok->valueType() : nullptr; + if (!vt) + return false; + return vt->isPrimitive(); +} + bool astIsIntegral(const Token *tok, bool unknown) { const ValueType *vt = tok ? tok->valueType() : nullptr; @@ -1950,6 +1958,9 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti return false; if (tok->isKeyword() && !isCPPCastKeyword(tok) && tok->str().compare(0,8,"operator") != 0) return false; + // A functional cast won't modify the variable + if (Token::Match(tok, "%type% (|{") && tok->tokType() == Token::eType && astIsPrimitive(tok->next())) + return false; const Token * parenTok = tok->next(); if (Token::simpleMatch(parenTok, "<") && parenTok->link()) parenTok = parenTok->link()->next(); diff --git a/lib/astutils.h b/lib/astutils.h index 483708b47..199794d9e 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -64,6 +64,7 @@ bool astHasToken(const Token* root, const Token * tok); bool astHasVar(const Token * tok, nonneg int varid); +bool astIsPrimitive(const Token* tok); /** Is expression a 'signed char' if no promotion is used */ bool astIsSignedChar(const Token *tok); /** Is expression a 'char' if no promotion is used? */ diff --git a/lib/checkio.cpp b/lib/checkio.cpp index ef8407a7a..5e597bc50 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -973,6 +973,10 @@ void CheckIO::checkFormatString(const Token * const tok, std::string specifier; bool done = false; while (!done) { + if (i == formatString.end()) { + done = true; + break; + } switch (*i) { case 's': if (argListTok->tokType() != Token::eString && @@ -1242,7 +1246,7 @@ void CheckIO::checkFormatString(const Token * const tok, case 'h': // Can be 'hh' (signed char or unsigned char) or 'h' (short int or unsigned short int) case 'l': { // Can be 'll' (long long int or unsigned long long int) or 'l' (long int or unsigned long int) // If the next character is the same (which makes 'hh' or 'll') then expect another alphabetical character - if (i != formatString.end() && (i + 1) != formatString.end() && *(i + 1) == *i) { + if ((i + 1) != formatString.end() && *(i + 1) == *i) { if ((i + 2) != formatString.end() && !isalpha(*(i + 2))) { std::string modifier; modifier += *i; @@ -1254,17 +1258,13 @@ void CheckIO::checkFormatString(const Token * const tok, specifier += *i++; } } else { - if (i != formatString.end()) { - if ((i + 1) != formatString.end() && !isalpha(*(i + 1))) { - std::string modifier; - modifier += *i; - invalidLengthModifierError(tok, numFormat, modifier); - done = true; - } else { - specifier = *i++; - } - } else { + if ((i + 1) != formatString.end() && !isalpha(*(i + 1))) { + std::string modifier; + modifier += *i; + invalidLengthModifierError(tok, numFormat, modifier); done = true; + } else { + specifier = *i++; } } } diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index 56927ee11..8e95f204e 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -263,6 +263,15 @@ struct ReverseTraversal { tok = parent; continue; } + if (tok->str() == "case") { + const Scope* scope = tok->scope(); + while (scope && scope->type != Scope::eSwitch) + scope = scope->nestedIn; + if (!scope || scope->type != Scope::eSwitch) + break; + tok = tok->tokAt(scope->bodyStart->index() - tok->index() - 1); + continue; + } if (!update(tok)) break; } diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index a03c18deb..f9c8a54c0 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -6504,6 +6504,8 @@ void SymbolDatabase::setValueTypeInTokenList(bool reportDebugWarnings, Token *to valuetype.sign = ValueType::Sign::UNSIGNED; else if (tok->previous()->isSigned()) valuetype.sign = ValueType::Sign::SIGNED; + else if (valuetype.isIntegral() && valuetype.type != ValueType::UNKNOWN_INT) + valuetype.sign = mDefaultSignedness; setValueType(tok, valuetype); } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fd4eaf1e8..36a07c1bb 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -356,7 +356,13 @@ static void combineValueProperties(const ValueFlow::Value &value1, const ValueFl static const Token *getCastTypeStartToken(const Token *parent) { // TODO: This might be a generic utility function? - if (!parent || parent->str() != "(") + if (!Token::Match(parent, "{|(")) + return nullptr; + // Functional cast + if (parent->isBinaryOp() && Token::Match(parent->astOperand1(), "%type% (|{") && + parent->astOperand1()->tokType() == Token::eType && astIsPrimitive(parent)) + return parent->astOperand1(); + if (parent->str() != "(") return nullptr; if (!parent->astOperand2() && Token::Match(parent,"( %name%")) return parent->next(); @@ -2328,6 +2334,9 @@ struct ValueFlowAnalyzer : Analyzer { } else if (getSettings()->library.getFunction(tok)) { // Assume library function doesn't modify user-global variables return Action::None; + // Function cast does not modify global variables + } else if (tok->tokType() == Token::eType && astIsPrimitive(tok->next())) { + return Action::None; } else { return Action::Invalid; } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index d20e928b8..15aeec1db 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -516,6 +516,12 @@ private: return values; } + std::list removeImpossible(std::list values) + { + values.remove_if(std::mem_fn(&ValueFlow::Value::isImpossible)); + return values; + } + void valueFlowNumber() { ASSERT_EQUALS(123, valueOfTok("x=123;", "123").intvalue); ASSERT_EQUALS_DOUBLE(192.0, valueOfTok("x=0x0.3p10;", "0x0.3p10").floatValue, 1e-5); // 3 * 16^-1 * 2^10 = 192 @@ -3015,7 +3021,22 @@ private: " return 0; \n" "}\n"; ASSERT_EQUALS(true, testValueOfX(code, 6U, 63)); - TODO_ASSERT_EQUALS(true, false, testValueOfXImpossible(code, 6U, 64)); + ASSERT_EQUALS(true, testValueOfXImpossible(code, 6U, 64)); + + code = "long long f(const long long& x, const long long& y) {\n" + " switch (s) {\n" + " case 0:\n" + " if (x >= 64)\n" + " return 0;\n" + " return long long{y} << long long{x};\n" + " case 1:\n" + " if (x >= 64) {\n" + " }\n" + " }\n" + " return 0; \n" + "}\n"; + ASSERT_EQUALS(true, testValueOfX(code, 6U, 63)); + ASSERT_EQUALS(true, testValueOfXImpossible(code, 6U, 64)); code = "int g(int x) { throw 0; }\n" "void f(int x) {\n"