Fix 10578: Value not impossible after check (#3549)

pfultz2 · web-flow · commit 035c70c441e7 · 2021-11-07T18:19:56.000+01:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -516,6 +516,12 @@ class TestValueFlow : public TestFixture {
         return values;
     }
 
+    std::list<ValueFlow::Value> removeImpossible(std::list<ValueFlow::Value> 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 @@ class TestValueFlow : public TestFixture {
                "    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"

Original file line number	Diff line number	Diff line change
`@@ -6504,6 +6504,8 @@ void SymbolDatabase::setValueTypeInTokenList(bool reportDebugWarnings, Token *to`
`6504`	`6504`	`valuetype.sign = ValueType::Sign::UNSIGNED;`
`6505`	`6505`	`else if (tok->previous()->isSigned())`
`6506`	`6506`	`valuetype.sign = ValueType::Sign::SIGNED;`
	`6507`	`+ else if (valuetype.isIntegral() && valuetype.type != ValueType::UNKNOWN_INT)`
	`6508`	`+ valuetype.sign = mDefaultSignedness;`
`6507`	`6509`	`setValueType(tok, valuetype);`
`6508`	`6510`	`}`
`6509`	`6511`