Fix issue 9404: False positive: Either the condition 'if(x)' is redundant or there is possible null pointer dereference: a->x (#2322)

* Fix issue 9404: False positive: Either the condition 'if(x)' is redundant or there is possible null pointer dereference: a->x

* Use simpleMatch

* Add a test case for the FP

* Check if expression is changed

* Check for no return scope

* Use simpleMatch

lib/astutils.cpp

@@ -952,7 +952,7 @@ static bool isEscapedOrJump(const Token* tok, bool functionsScope)
         return Token::Match(tok, "return|goto|throw|continue|break");
 }
 
-bool isReturnScope(const Token * const endToken, const Settings * settings, bool functionScope)
+bool isReturnScope(const Token * const endToken, const Library * library, bool functionScope)
 {
     if (!endToken || endToken->str() != "}")
         return false;
@@ -965,7 +965,7 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool
 
     if (Token::simpleMatch(prev, "}")) {
         if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {"))
-            return isReturnScope(prev, settings, functionScope) && isReturnScope(prev->link()->tokAt(-2), settings, functionScope);
+            return isReturnScope(prev, library, functionScope) && isReturnScope(prev->link()->tokAt(-2), library, functionScope);
         if (Token::simpleMatch(prev->link()->previous(), ") {") &&
             Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") &&
             !Token::findsimplematch(prev->link(), "break", prev)) {
@@ -974,7 +974,7 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool
         if (isEscaped(prev->link()->astTop(), functionScope))
             return true;
         if (Token::Match(prev->link()->previous(), "[;{}] {"))
-            return isReturnScope(prev, settings, functionScope);
+            return isReturnScope(prev, library, functionScope);
     } else if (Token::simpleMatch(prev, ";")) {
         if (Token::simpleMatch(prev->previous(), ") ;") && Token::Match(prev->linkAt(-1)->tokAt(-2), "[;{}] %name% (")) {
             const Token * ftok = prev->linkAt(-1)->previous();
@@ -984,8 +984,8 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool
                     return true;
                 if (function->isAttributeNoreturn())
                     return true;
-            } else if (settings) {
+            } else if (library) {
-                if (settings->library.isnoreturn(ftok))
+                if (library->isnoreturn(ftok))
                     return true;
             }
             return false;
@@ -1642,6 +1642,40 @@ struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const
                 return Result(Result::Type::BAILOUT);
         }
 
+        if (mWhat == What::ValueFlow && Token::simpleMatch(tok, "if (") && Token::simpleMatch(tok->linkAt(1), ") {")) {
+            const Token *bodyStart = tok->linkAt(1)->next();
+            const Token *conditionStart = tok->next();
+            const Token *condTok = conditionStart->astOperand2();
+            if (condTok->hasKnownIntValue()) {
+                bool cond = condTok->values().front().intvalue;
+                if (cond) {
+                    FwdAnalysis::Result result = checkRecursive(expr, bodyStart, bodyStart->link(), exprVarIds, local, true, depth);
+                    if (result.type != Result::Type::NONE)
+                        return result;
+                } else if (Token::simpleMatch(bodyStart->link(), "} else {")) {
+                    bodyStart = bodyStart->tokAt(2);
+                    FwdAnalysis::Result result = checkRecursive(expr, bodyStart, bodyStart->link(), exprVarIds, local, true, depth);
+                    if (result.type != Result::Type::NONE)
+                        return result;
+                }
+            }
+            tok = bodyStart->link();
+            if (isReturnScope(tok, &mLibrary))
+                return Result(Result::Type::BAILOUT);
+            if (Token::simpleMatch(tok, "} else {"))
+                tok = tok->linkAt(2);
+            if (!tok)
+                return Result(Result::Type::BAILOUT);
+
+            // Is expr changed in condition?
+            if (!isUnchanged(conditionStart, conditionStart->link(), exprVarIds, local))
+                return Result(Result::Type::BAILOUT);
+
+            // Is expr changed in condition body?
+            if (!isUnchanged(bodyStart, bodyStart->link(), exprVarIds, local))
+                return Result(Result::Type::BAILOUT);
+        }
+
         if (!local && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) {
             // TODO: this is a quick bailout
             return Result(Result::Type::BAILOUT);

lib/astutils.h

@@ -126,7 +126,7 @@ bool isWithoutSideEffects(bool cpp, const Token* tok);
 bool isUniqueExpression(const Token* tok);
 
 /** Is scope a return scope (scope will unconditionally return) */
-bool isReturnScope(const Token *endToken, const Settings * settings = nullptr, bool functionScope=false);
+bool isReturnScope(const Token * const endToken, const Library * library=nullptr, bool functionScope=false);
 
 /// Return the token to the function and the argument number
 const Token * getTokenArgumentFunction(const Token * tok, int& argn);

lib/symboldatabase.cpp

@@ -1401,7 +1401,7 @@ void SymbolDatabase::createSymbolDatabaseEscapeFunctions()
         Function * function = scope.function;
         if (!function)
             continue;
-        function->isEscapeFunction(isReturnScope(scope.bodyEnd, mSettings, true));
+        function->isEscapeFunction(isReturnScope(scope.bodyEnd, &mSettings->library, true));
     }
 }
 

lib/templatesimplifier.cpp

@@ -723,6 +723,8 @@ bool TemplateSimplifier::getTemplateDeclarations()
         }
         if (!tok1)
             syntaxError(tok);
+        if (!tok1->next())
+            syntaxError(tok);
         // Some syntax checks, see #6865
         if (!tok->tokAt(2))
             syntaxError(tok->next());

lib/valueflow.cpp

@@ -1678,9 +1678,12 @@ static void valueFlowReverse(TokenList *tokenlist,
             if (Token::Match(tok2->previous(), "!!* %name% =")) {
                 Token* assignTok = const_cast<Token*>(tok2->next()->astOperand2());
                 if (!assignTok->hasKnownValue()) {
-                    std::list<ValueFlow::Value> values = {val};
                     setTokenValue(assignTok, val, settings);
+                    const std::string info = "Assignment from '" + assignTok->expressionString() + "'";
+                    val.errorPath.emplace_back(assignTok, info);
+                    std::list<ValueFlow::Value> values = {val};
                     if (val2.condition) {
+                        val2.errorPath.emplace_back(assignTok, info);
                         setTokenValue(assignTok, val2, settings);
                         values.push_back(val2);
                     }
@@ -2190,7 +2193,7 @@ static bool valueFlowForwardVariable(Token* const startToken,
             ++indentlevel;
         else if (indentlevel >= 0 && tok2->str() == "}") {
             --indentlevel;
-            if (indentlevel <= 0 && isReturnScope(tok2, settings) && Token::Match(tok2->link()->previous(), "else|) {")) {
+            if (indentlevel <= 0 && isReturnScope(tok2, &settings->library) && Token::Match(tok2->link()->previous(), "else|) {")) {
                 const Token *condition = tok2->link();
                 const bool iselse = Token::simpleMatch(condition->tokAt(-2), "} else {");
                 if (iselse)
@@ -2240,7 +2243,7 @@ static bool valueFlowForwardVariable(Token* const startToken,
                     return true;
             } else if (indentlevel <= 0 &&
                        Token::simpleMatch(tok2->link()->previous(), "else {") &&
-                       !isReturnScope(tok2->link()->tokAt(-2), settings) &&
+                       !isReturnScope(tok2->link()->tokAt(-2), &settings->library) &&
                        isVariableChanged(tok2->link(), tok2, varid, var->isGlobal(), settings, tokenlist->isCPP())) {
                 lowerToPossible(values);
             }
@@ -2506,7 +2509,7 @@ static bool valueFlowForwardVariable(Token* const startToken,
             }
 
             // stop after conditional return scopes that are executed
-            if (isReturnScope(end, settings)) {
+            if (isReturnScope(end, &settings->library)) {
                 std::list<ValueFlow::Value>::iterator it;
                 for (it = values.begin(); it != values.end();) {
                     if (conditionIsTrue(tok2->next()->astOperand2(), getProgramMemory(tok2, varid, *it)))
@@ -4375,7 +4378,7 @@ struct ValueFlowConditionHandler {
                             continue;
                         }
 
-                        bool dead_if = isReturnScope(after, settings) ||
+                        bool dead_if = isReturnScope(after, &settings->library) ||
                                        (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (") &&
                                         !isBreakScope(after));
                         bool dead_else = false;
@@ -4387,7 +4390,7 @@ struct ValueFlowConditionHandler {
                                     bailout(tokenlist, errorLogger, after, "possible noreturn scope");
                                 continue;
                             }
-                            dead_else = isReturnScope(after, settings);
+                            dead_else = isReturnScope(after, &settings->library);
                         }
 
                         if (dead_if && dead_else)

test/testnullpointer.cpp

@@ -82,7 +82,9 @@ private:
         TEST_CASE(nullpointer40);
         TEST_CASE(nullpointer41);
         TEST_CASE(nullpointer42);
+        TEST_CASE(nullpointer43); // #9404
         TEST_CASE(nullpointer44); // #9395, #9423
+        TEST_CASE(nullpointer45);
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -1538,6 +1540,17 @@ private:
             errout.str());
     }
 
+    void nullpointer43() {
+        check("struct A { int* x; };\n"
+              "void f(A* a) {\n"
+              "    int * x = a->x;\n"
+              "    if (x) {\n"
+              "        (void)*a->x;\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer44() {
         // #9395
         check("int foo( ) {\n"
@@ -1567,6 +1580,39 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer45() {
+        check("struct a {\n"
+              "  a *b() const;\n"
+              "};\n"
+              "void g() { throw 0; }\n"
+              "a h(a * c) {\n"
+              "  if (c && c->b()) {}\n"
+              "  if (!c)\n"
+              "    g();\n"
+              "  if (!c->b())\n"
+              "    g();\n"
+              "  a d = *c->b();\n"
+              "  return d;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct a {\n"
+              "  a *b() const;\n"
+              "};\n"
+              "void e() { throw 0; }\n"
+              "a f() {\n"
+              "  a *c = 0;\n"
+              "  if (0 && c->b()) {}\n"
+              "  if (!c)\n"
+              "    e();\n"
+              "  if (!c->b())\n"
+              "    e();\n"
+              "  a d = *c->b();\n"
+              "  return d;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"