Fix 12247: FP uninitvar with pointer to uninitialized array (#6414)

danmar · May 21, 2024 · 9fb2625 · 9fb2625
1 parent 9990975
commit 9fb2625
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 13 deletions.
diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -2435,6 +2435,31 @@ static bool isArray(const Token* tok)
     return false;
 }
 
+static bool isMutableExpression(const Token* tok)
+{
+    if (!tok)
+        return false;
+    if (tok->isLiteral() || tok->isKeyword() || tok->isStandardType() || tok->isEnumerator())
+        return false;
+    if (Token::Match(tok, ",|;|:|]|)|}"))
+        return false;
+    if (Token::simpleMatch(tok, "[ ]"))
+        return false;
+    if (Token::Match(tok->previous(), "%name% (") && tok->previous()->isKeyword())
+        return false;
+    if (Token::Match(tok, "<|>") && tok->link())
+        return false;
+    if (Token::simpleMatch(tok, "[") && tok->astOperand1())
+        return isMutableExpression(tok->astOperand1());
+    if (const Variable* var = tok->variable()) {
+        if (var->nameToken() == tok)
+            return false;
+        if (!var->isPointer() && var->isConst())
+            return false;
+    }
+    return true;
+}
+
 bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings &settings, bool *inconclusive)
 {
     if (!tok)
@@ -2545,7 +2570,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
 
 bool isVariableChanged(const Token *tok, int indirect, const Settings &settings, int depth)
 {
-    if (!tok)
+    if (!isMutableExpression(tok))
         return false;
 
     if (indirect == 0 && isConstVarExpression(tok))
@@ -2594,12 +2619,12 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings &settings,
     tok2 = skipRedundantPtrOp(tok2, tok2->astParent());
 
     if (tok2->astParent() && tok2->astParent()->isAssignmentOp()) {
-        if (((indirect == 0 || tok2 != tok) || (indirect == 1 && tok2->str() == ".")) && tok2 == tok2->astParent()->astOperand1())
+        if (astIsLHS(tok2))
             return true;
         // Check if assigning to a non-const lvalue
         const Variable * var = getLHSVariable(tok2->astParent());
-        if (var && var->isReference() && !var->isConst() &&
-            ((var->nameToken() && var->nameToken()->next() == tok2->astParent()) || var->isPointer())) {
+        if (var && var->isReference() && !var->isConst() && var->nameToken() &&
+            var->nameToken()->next() == tok2->astParent()) {
             if (!var->isLocal() || isVariableChanged(var, settings, depth - 1))
                 return true;
         }
@@ -2815,7 +2840,7 @@ static bool isExpressionChangedAt(const F& getExprTok,
 {
     if (depth < 0)
         return true;
-    if (tok->isLiteral() || tok->isKeyword() || tok->isStandardType() || Token::Match(tok, ",|;|:"))
+    if (!isMutableExpression(tok))
         return false;
     if (tok->exprId() != exprid || (!tok->varId() && !tok->isName())) {
         if (globalvar && Token::Match(tok, "%name% (") && !(tok->function() && tok->function()->isAttributePure()))

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -1458,6 +1458,24 @@ static const Token* getVariableChangedStart(const Variable* p)
     return start;
 }
 
+static bool isConstPointerVariable(const Variable* p, const Settings& settings)
+{
+    const int indirect = p->isArray() ? p->dimensions().size() : 1;
+    const Token* start = getVariableChangedStart(p);
+    while (const Token* tok =
+               findVariableChanged(start, p->scope()->bodyEnd, indirect, p->declarationId(), false, settings)) {
+        if (p->isReference())
+            return false;
+        // Assigning a pointer through another pointer may still be const
+        if (!Token::simpleMatch(tok->astParent(), "="))
+            return false;
+        if (!astIsLHS(tok))
+            return false;
+        start = tok->next();
+    }
+    return true;
+}
+
 namespace {
     struct CompareVariables {
         bool operator()(const Variable* a, const Variable* b) const {
@@ -1500,6 +1518,9 @@ void CheckOther::checkConstPointer()
                 !astIsRangeBasedForDecl(nameTok))
                 continue;
         }
+        // Skip function pointers
+        if (Token::Match(nameTok, "%name% ) ("))
+            continue;
         const ValueType* const vt = tok->valueType();
         if (!vt)
             continue;
@@ -1609,9 +1630,11 @@ void CheckOther::checkConstPointer()
                 continue;
         }
         if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), p) == nonConstPointers.cend()) {
-            const Token *start = getVariableChangedStart(p);
-            const int indirect = p->isArray() ? p->dimensions().size() : 1;
-            if (isVariableChanged(start, p->scope()->bodyEnd, indirect, p->declarationId(), false, *mSettings))
+            // const Token *start = getVariableChangedStart(p);
+            // const int indirect = p->isArray() ? p->dimensions().size() : 1;
+            // if (isVariableChanged(start, p->scope()->bodyEnd, indirect, p->declarationId(), false, *mSettings))
+            //     continue;
+            if (!isConstPointerVariable(p, *mSettings))
                 continue;
             if (p->typeStartToken() && p->typeStartToken()->isSimplifiedTypedef() && !(Token::simpleMatch(p->typeEndToken(), "*") && !p->typeEndToken()->isSimplifiedTypedef()))
                 continue;

diff --git a/test/testastutils.cpp b/test/testastutils.cpp
@@ -249,7 +249,6 @@ class TestAstUtils : public TestFixture {
                                         "void f(int x) { g(&x); }\n",
                                         "{",
                                         "}"));
-
         ASSERT_EQUALS(false, isVariableChanged("const int A[] = { 1, 2, 3 };", "[", "]"));
     }
 

diff --git a/test/teststl.cpp b/test/teststl.cpp
@@ -2429,29 +2429,29 @@ class TestStl : public TestFixture {
               "}");
         ASSERT_EQUALS("[test.cpp:3]: (error) When ii==foo.size(), foo.at(ii) is out of bounds.\n", errout_str());
 
-        check("void foo(const std::string& foo) {\n"
+        check("void foo(std::string& foo) {\n"
               "    for (unsigned int ii = 0; ii <= foo.length(); ++ii) {\n"
               "       foo[ii] = 'x';\n"
               "    }\n"
               "}");
         ASSERT_EQUALS("[test.cpp:3]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout_str());
 
-        check("void foo(const std::string& foo, unsigned int ii) {\n"
+        check("void foo(std::string& foo, unsigned int ii) {\n"
               "    if (ii <= foo.length()) {\n"
               "       foo[ii] = 'x';\n"
               "    }\n"
               "}");
         ASSERT_EQUALS("[test.cpp:3]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout_str());
 
-        check("void foo(const std::string& foo, unsigned int ii) {\n"
+        check("void foo(std::string& foo, unsigned int ii) {\n"
               "    do {\n"
               "       foo[ii] = 'x';\n"
               "       ++i;\n"
               "    } while(ii <= foo.length());\n"
               "}");
         ASSERT_EQUALS("[test.cpp:3]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout_str());
 
-        check("void foo(const std::string& foo, unsigned int ii) {\n"
+        check("void foo(std::string& foo, unsigned int ii) {\n"
               "    if (anything()) {\n"
               "    } else if (ii <= foo.length()) {\n"
               "       foo[ii] = 'x';\n"

diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp
@@ -6492,6 +6492,22 @@ class TestUninitVar : public TestFixture {
                         "    f(i);\n"
                         "}\n");
         ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (warning) Uninitialized variable: r\n", errout_str());
+
+        // #12247
+        valueFlowUninit("void f() {\n"
+                        "    char a[10], *p = &a[0];\n"
+                        "    p = getenv(\"abc\");\n"
+                        "    printf(\"%\", p);\n"
+                        "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        valueFlowUninit("void f(char *q) {\n"
+                        "    char a[1];\n"
+                        "    char *p = a;\n"
+                        "    p = q;\n"
+                        "    printf(\"%s\", p);\n"
+                        "}\n");
+        ASSERT_EQUALS("", errout_str());
     }
 
     void valueFlowUninitBreak() { // Do not show duplicate warnings about the same uninitialized value