From 12e731ad494a12e12d8a0ee0177d6e5dfddd0ae4 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 25 Nov 2021 15:34:00 -0600 Subject: [PATCH] Fix 10605: FP containerOutOfBounds with empty() check (#3572) --- lib/astutils.cpp | 33 +++++++++++++++++++++---- lib/astutils.h | 2 ++ lib/reverseanalyzer.cpp | 52 +++++++++++++++++++++++++++++++++++++++- lib/valueflow.cpp | 37 ++++++++++++++++++---------- test/testnullpointer.cpp | 20 ++++++++++++++++ test/teststl.cpp | 8 +++++++ 6 files changed, 134 insertions(+), 18 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 5fc26cd1a..29586cc69 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -53,10 +53,10 @@ void visitAstNodesGeneric(T *ast, std::function visitor) if (c == ChildrenToVisit::done) break; - if (c == ChildrenToVisit::op1 || c == ChildrenToVisit::op1_and_op2) - tokens.push(tok->astOperand1()); if (c == ChildrenToVisit::op2 || c == ChildrenToVisit::op1_and_op2) tokens.push(tok->astOperand2()); + if (c == ChildrenToVisit::op1 || c == ChildrenToVisit::op1_and_op2) + tokens.push(tok->astOperand1()); } } @@ -734,6 +734,8 @@ static bool isInLoopCondition(const Token * tok) /// If tok2 comes after tok1 bool precedes(const Token * tok1, const Token * tok2) { + if (tok1 == tok2) + return false; if (!tok1) return false; if (!tok2) @@ -741,6 +743,18 @@ bool precedes(const Token * tok1, const Token * tok2) return tok1->index() < tok2->index(); } +/// If tok1 comes after tok2 +bool succedes(const Token* tok1, const Token* tok2) +{ + if (tok1 == tok2) + return false; + if (!tok1) + return false; + if (!tok2) + return true; + return tok1->index() > tok2->index(); +} + bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive) { if (tok->varId() == varid) @@ -1865,11 +1879,12 @@ bool isScopeBracket(const Token* tok) return false; } -const Token * getTokenArgumentFunction(const Token * tok, int& argn) +template )> +T* getTokenArgumentFunctionImpl(T* tok, int& argn) { argn = -1; { - const Token *parent = tok->astParent(); + T* parent = tok->astParent(); if (parent && parent->isUnaryOp("&")) parent = parent->astParent(); while (parent && parent->isCast()) @@ -1891,7 +1906,7 @@ const Token * getTokenArgumentFunction(const Token * tok, int& argn) return nullptr; } - const Token* argtok = tok; + T* argtok = tok; while (argtok && argtok->astParent() && (!Token::Match(argtok->astParent(), ",|(|{") || argtok->astParent()->isCast())) { argtok = argtok->astParent(); } @@ -1935,6 +1950,14 @@ const Token * getTokenArgumentFunction(const Token * tok, int& argn) return tok; } +const Token* getTokenArgumentFunction(const Token* tok, int& argn) { + return getTokenArgumentFunctionImpl(tok, argn); +} + +Token* getTokenArgumentFunction(Token* tok, int& argn) { + return getTokenArgumentFunctionImpl(tok, argn); +} + std::vector getArgumentVars(const Token* tok, int argnr) { std::vector result; diff --git a/lib/astutils.h b/lib/astutils.h index 538e1846e..f7597cebd 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -151,6 +151,7 @@ bool extractForLoopValues(const Token *forToken, long long * const lastValue); bool precedes(const Token * tok1, const Token * tok2); +bool succedes(const Token* tok1, const Token* tok2); bool exprDependsOnThis(const Token* expr, bool onVar = true, nonneg int depth = 0); @@ -208,6 +209,7 @@ bool isReturnScope(const Token* const endToken, /// Return the token to the function and the argument number const Token * getTokenArgumentFunction(const Token * tok, int& argn); +Token* getTokenArgumentFunction(Token* tok, int& argn); std::vector getArgumentVars(const Token* tok, int argnr); diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index 08c35bf02..f16107617 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -40,6 +40,44 @@ struct ReverseTraversal { return true; } + Token* getParentFunction(Token* tok) + { + if (!tok) + return nullptr; + if (!tok->astParent()) + return nullptr; + int argn = -1; + if (Token* ftok = getTokenArgumentFunction(tok, argn)) { + while (!Token::Match(ftok, "(|{")) { + if (!ftok) + return nullptr; + if (ftok->index() >= tok->index()) + return nullptr; + if (ftok->link()) + ftok = ftok->link()->next(); + else + ftok = ftok->next(); + } + if (ftok == tok) + return nullptr; + return ftok; + } + return nullptr; + } + + Token* getTopFunction(Token* tok) + { + if (!tok) + return nullptr; + if (!tok->astParent()) + return tok; + Token* parent = tok; + Token* top = tok; + while ((parent = getParentFunction(parent))) + top = parent; + return top; + } + bool updateRecursive(Token* start) { bool continueB = true; visitAstNodes(start, [&](Token* tok) { @@ -121,7 +159,7 @@ struct ReverseTraversal { if (start == end) return; std::size_t i = start->index(); - for (Token* tok = start->previous(); tok != end; tok = tok->previous()) { + for (Token* tok = start->previous(); succedes(tok, end); tok = tok->previous()) { if (tok->index() >= i) throw InternalError(tok, "Cyclic reverse analysis."); i = tok->index(); @@ -198,6 +236,16 @@ struct ReverseTraversal { tok = previousBeforeAstLeftmostLeaf(assignTop)->next(); continue; } + if (tok->str() == ")" && !isUnevaluated(tok)) { + if (Token* top = getTopFunction(tok->link())) { + if (!updateRecursive(top)) + break; + Token* next = previousBeforeAstLeftmostLeaf(top); + if (next && precedes(next, tok)) + tok = next->next(); + } + continue; + } if (tok->str() == "}") { Token* condTok = getCondTokFromEnd(tok); if (!condTok) @@ -283,6 +331,8 @@ struct ReverseTraversal { } static Token* assignExpr(Token* tok) { + if (Token::Match(tok, ")|}")) + tok = tok->link(); while (tok->astParent() && (astIsRHS(tok) || !tok->astParent()->isBinaryOp())) { if (tok->astParent()->isAssignmentOp()) return tok->astParent(); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 4642bb392..e2b6e6fc8 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2115,6 +2115,23 @@ struct ValueFlowAnalyzer : Analyzer { return Action::None; } + Action isGlobalModified(const Token* tok) const + { + if (tok->function()) { + if (!tok->function()->isConstexpr() && !isConstFunctionCall(tok, getSettings()->library)) + return Action::Invalid; + } 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 if (Token::Match(tok, "%name% (")) { + return Action::Invalid; + } + return Action::None; + } + static const std::string& getAssign(const Token* tok, Direction d) { if (d == Direction::Forward) @@ -2245,6 +2262,11 @@ struct ValueFlowAnalyzer : Analyzer { Action analyzeMatch(const Token* tok, Direction d) const { const Token* parent = tok->astParent(); + if (d == Direction::Reverse && isGlobal() && !dependsOnThis() && Token::Match(parent, ". %name% (")) { + Action a = isGlobalModified(parent->next()); + if (a != Action::None) + return a; + } if ((astIsPointer(tok) || astIsSmartPointer(tok)) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) return Action::Read; @@ -2328,18 +2350,7 @@ struct ValueFlowAnalyzer : Analyzer { // bailout: global non-const variables if (isGlobal() && !dependsOnThis() && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { - if (tok->function()) { - if (!tok->function()->isConstexpr() && !isConstFunctionCall(tok, getSettings()->library)) - return Action::Invalid; - } 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; - } + return isGlobalModified(tok); } return Action::None; } @@ -4998,6 +5009,8 @@ struct ConditionHandler { for (Token *tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { if (Token::Match(tok, "if|while|for (")) continue; + if (Token::Match(tok, ":|;|,")) + continue; const Token* top = tok->astTop(); if (!top) diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 8716801c6..e43fd237c 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -125,6 +125,7 @@ private: TEST_CASE(nullpointer83); // #9870 TEST_CASE(nullpointer84); // #9873 TEST_CASE(nullpointer85); // #10210 + TEST_CASE(nullpointer86); TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -2547,6 +2548,25 @@ private: errout.str()); } + void nullpointer86() + { + check("struct A {\n" + " A* a() const;\n" + " int b() const;\n" + "};\n" + "A* f(A* t) {\n" + " if (t->b() == 0) {\n" + " return t;\n" + " }\n" + " return t->a();\n" + "}\n" + "void g(A* t) {\n" + " t = f(t->a());\n" + " if (!t->a()) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n" diff --git a/test/teststl.cpp b/test/teststl.cpp index e76e532ec..719ea3bbc 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -646,6 +646,14 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); + checkNormal("void f(std::vector& v, int i) {\n" + " if (i > -1) {\n" + " v.erase(v.begin() + i);\n" + " if (v.empty()) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + checkNormal("void g(const char *, ...) { exit(1); }\n" // #10025 "void f(const char c[]) {\n" " std::vector v = get();\n"