From 838b6b86e3dbeab6e525039bc5f9020587595d01 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Tue, 8 Aug 2023 10:38:03 +0200 Subject: [PATCH] Fix #11862 FN constParameterPointer with increment (#5291) --- lib/astutils.cpp | 19 ++++++++++++------- lib/checkother.cpp | 13 +++++++++---- lib/checkunusedvar.cpp | 4 ++-- lib/valueflow.cpp | 25 +++++++++++++++++-------- test/testother.cpp | 8 +++++++- test/teststl.cpp | 29 +++++++++++------------------ 6 files changed, 58 insertions(+), 40 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 94307ab27..ca7c08af3 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2188,7 +2188,7 @@ T* getTokenArgumentFunctionImpl(T* tok, int& argn) argn = -1; { T* parent = tok->astParent(); - if (parent && parent->isUnaryOp("&")) + if (parent && (parent->isUnaryOp("&") || parent->isIncDecOp())) parent = parent->astParent(); while (parent && parent->isCast()) parent = parent->astParent(); @@ -2196,7 +2196,7 @@ T* getTokenArgumentFunctionImpl(T* tok, int& argn) parent = parent->astParent(); // passing variable to subfunction? - if (Token::Match(parent, "[[(,{]")) + if (Token::Match(parent, "[*[(,{]")) ; else if (Token::simpleMatch(parent, ":")) { while (Token::Match(parent, "[?:]")) @@ -2347,6 +2347,10 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti if (addressOf) indirect++; + const bool deref = tok->astParent() && tok->astParent()->isUnaryOp("*"); + if (deref && indirect > 0) + indirect--; + int argnr; tok = getTokenArgumentFunction(tok, argnr); if (!tok) @@ -2455,7 +2459,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, (Token::simpleMatch(tok2->astParent(), ":") && Token::simpleMatch(tok2->astParent()->astParent(), "?"))) tok2 = tok2->astParent(); - if (tok2->astParent() && tok2->astParent()->tokType() == Token::eIncDecOp) + if (indirect == 0 && tok2->astParent() && tok2->astParent()->tokType() == Token::eIncDecOp) return true; auto skipRedundantPtrOp = [](const Token* tok, const Token* parent) { @@ -2578,7 +2582,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, const Token *parent = tok2->astParent(); while (Token::Match(parent, ".|::")) parent = parent->astParent(); - if (parent && parent->tokType() == Token::eIncDecOp) + if (parent && parent->tokType() == Token::eIncDecOp && (indirect == 0 || tok2 != tok)) return true; // structured binding, nonconst reference variable in lhs @@ -2615,7 +2619,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, if (indirect > 0) { // check for `*(ptr + 1) = new_value` case parent = tok2->astParent(); - while (parent && parent->isArithmeticalOp() && parent->isBinaryOp()) { + while (parent && ((parent->isArithmeticalOp() && parent->isBinaryOp()) || parent->isIncDecOp())) { parent = parent->astParent(); } if (Token::simpleMatch(parent, "*")) { @@ -2839,8 +2843,9 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end if (vt->type == ValueType::ITERATOR) ++indirect; } - if (isExpressionChangedAt(tok, tok2, indirect, global, settings, cpp, depth)) - return true; + for (int i = 0; i <= indirect; ++i) + if (isExpressionChangedAt(tok, tok2, i, global, settings, cpp, depth)) + return true; } } return false; diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cf421268b..f11a523a9 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1587,9 +1587,10 @@ void CheckOther::checkConstPointer() if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), var) != nonConstPointers.cend()) continue; pointers.emplace_back(var); - const Token* const parent = tok->astParent(); + const Token* parent = tok->astParent(); enum Deref { NONE, DEREF, MEMBER } deref = NONE; - if (parent && parent->isUnaryOp("*")) + bool hasIncDec = false; + if (parent && (parent->isUnaryOp("*") || (hasIncDec = parent->isIncDecOp() && parent->astParent() && parent->astParent()->isUnaryOp("*")))) deref = DEREF; else if (Token::simpleMatch(parent, "[") && parent->astOperand1() == tok && tok != nameTok) deref = DEREF; @@ -1600,7 +1601,7 @@ void CheckOther::checkConstPointer() else if (astIsRangeBasedForDecl(tok)) continue; if (deref != NONE) { - const Token* const gparent = parent->astParent(); + const Token* gparent = parent->astParent(); if (deref == MEMBER) { if (!gparent) continue; @@ -1613,6 +1614,10 @@ void CheckOther::checkConstPointer() } if (Token::Match(gparent, "%cop%") && !gparent->isUnaryOp("&") && !gparent->isUnaryOp("*")) continue; + if (hasIncDec) { + parent = gparent; + gparent = gparent ? gparent->astParent() : nullptr; + } int argn = -1; if (Token::simpleMatch(gparent, "return")) { const Function* function = gparent->scope()->function; @@ -1633,7 +1638,7 @@ void CheckOther::checkConstPointer() continue; else if (const Token* ftok = getTokenArgumentFunction(parent, argn)) { bool inconclusive{}; - if (!isVariableChangedByFunctionCall(ftok, vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) + if (!isVariableChangedByFunctionCall(ftok->next(), vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) continue; } } else { diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index bd42a6182..9f0ea5dc3 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1691,8 +1691,8 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To } // check if global variable is changed if (bodyVariable->isGlobal() || (pointersToGlobals.find(bodyVariable) != pointersToGlobals.end())) { - const int depth = 20; - if (isVariableChanged(bodyToken, depth, mSettings, mTokenizer->isCPP())) { + const int indirect = bodyVariable->isArray() ? bodyVariable->dimensions().size() : bodyVariable->isPointer(); + if (isVariableChanged(bodyToken, indirect, mSettings, mTokenizer->isCPP())) { return false; } // check if pointer to global variable assigned to another variable (another_var = &global_var) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b6601ce6b..dcadeaee9 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2564,19 +2564,22 @@ struct ValueFlowAnalyzer : Analyzer { return read; } - virtual Action isAliasModified(const Token* tok) const { + virtual Action isAliasModified(const Token* tok, int indirect = -1) const { // Lambda function call if (Token::Match(tok, "%var% (")) // TODO: Check if modified in the lambda function return Action::Invalid; - int indirect = 0; - if (const ValueType* vt = tok->valueType()) { - indirect = vt->pointer; - if (vt->type == ValueType::ITERATOR) - ++indirect; + if (indirect == -1) { + indirect = 0; + if (const ValueType* vt = tok->valueType()) { + indirect = vt->pointer; + if (vt->type == ValueType::ITERATOR) + ++indirect; + } } - if (isVariableChanged(tok, indirect, getSettings(), isCPP())) - return Action::Invalid; + for (int i = 0; i <= indirect; ++i) + if (isVariableChanged(tok, i, getSettings(), isCPP())) + return Action::Invalid; return Action::None; } @@ -3213,6 +3216,12 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { bool isVariable() const override { return expr->varId() > 0; } + + Action isAliasModified(const Token* tok, int indirect) const override { + if (value.isSymbolicValue() && tok->exprId() == value.tokvalue->exprId()) + indirect = 0; + return SingleValueFlowAnalyzer::isAliasModified(tok, indirect); + } }; struct SameExpressionAnalyzer : ExpressionAnalyzer { diff --git a/test/testother.cpp b/test/testother.cpp index cc2be93e0..eb1b30012 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3800,6 +3800,12 @@ private: " return strlen(p);\n" "}\n"); ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'p' can be declared as pointer to const\n", errout.str()); + + check("void f(int* p) {\n" // #11862 + " long long j = *(p++);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'p' can be declared as pointer to const\n", + errout.str()); } void switchRedundantAssignmentTest() { @@ -8128,7 +8134,7 @@ private: "void packed_object_info(struct object_info *oi) {\n" " if (*oi->typep < 0);\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'oi' can be declared as pointer to const\n", errout.str()); } void checkSuspiciousSemicolon1() { diff --git a/test/teststl.cpp b/test/teststl.cpp index b25a44431..bb9f3ad7b 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -2819,27 +2819,20 @@ private: } void eraseOnVector() { - check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.begin();\n" - " m_ImplementationMap.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector - " m_ImplementationMap.erase(aIt);\n" + check("void f(std::vector& v) {\n" + " std::vector::iterator aIt = v.begin();\n" + " v.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector + " v.erase(aIt);\n" "}"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str()); - check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.begin();\n" - " m_ImplementationMap.erase(*aIt);\n" // All iterators become invalidated when erasing from std::vector - " m_ImplementationMap.erase(aIt);\n" + check("void f(std::vector& v) {\n" + " std::vector::iterator aIt = v.begin();\n" + " std::vector::iterator bIt = v.begin();\n" + " v.erase(bIt);\n" // All iterators become invalidated when erasing from std::vector + " aIt = v.erase(aIt);\n" "}"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); - - check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.begin();\n" - " std::vector::iterator bIt = m_ImplementationMap.begin();\n" - " m_ImplementationMap.erase(*bIt);\n" // All iterators become invalidated when erasing from std::vector - " aIt = m_ImplementationMap.erase(aIt);\n" - "}"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str()); } void pushback1() {