From 9fb26258281af1a45086cf757a07e5089fc583e6 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 21 May 2024 07:29:17 -0500 Subject: [PATCH] Fix 12247: FP uninitvar with pointer to uninitialized array (#6414) --- lib/astutils.cpp | 35 ++++++++++++++++++++++++++++++----- lib/checkother.cpp | 29 ++++++++++++++++++++++++++--- test/testastutils.cpp | 1 - test/teststl.cpp | 8 ++++---- test/testuninitvar.cpp | 16 ++++++++++++++++ 5 files changed, 76 insertions(+), 13 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 35a375d414d..0e5902e1014 100644 --- 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 index 2ec02e038ed..2abe8d7153c 100644 --- 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 index 01d66d01614..8e54c5a6f3d 100644 --- 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 index 3ee3ab2b508..f3d7b941f82 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -2429,21 +2429,21 @@ 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" @@ -2451,7 +2451,7 @@ class TestStl : public TestFixture { "}"); 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 index 08335e8cbeb..7aea86d72ae 100644 --- 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