Fix 10605: FP containerOutOfBounds with empty() check (#3572)

lib/astutils.cpp

@@ -53,10 +53,10 @@ void visitAstNodesGeneric(T *ast, std::function<ChildrenToVisit(T *)> 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<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
+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<const Variable*> getArgumentVars(const Token* tok, int argnr)
 {
     std::vector<const Variable*> result;

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<const Variable*> getArgumentVars(const Token* tok, int argnr);
 

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();

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()) {
+            return isGlobalModified(tok);
-                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 Action::None;
     }
@@ -4998,6 +5009,8 @@ struct ConditionHandler {
             for (Token *tok = const_cast<Token *>(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)

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"

test/teststl.cpp

@@ -646,6 +646,14 @@ private:
                     "}\n");
         ASSERT_EQUALS("", errout.str());
 
+        checkNormal("void f(std::vector<int>& 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<int> v = get();\n"