Fix 9873: False negative: null pointer when checking raw pointer (#3485)

lib/astutils.cpp

@@ -1525,6 +1525,18 @@ bool isConstFunctionCall(const Token* ftok, const Library& library)
         } else if (lf->argumentChecks.empty()) {
             return false;
         }
+    } else if (Token::Match(ftok->previous(), ". %name% (") && ftok->previous()->originalName() != "->" &&
+               astIsSmartPointer(ftok->previous()->astOperand1())) {
+        return Token::Match(ftok, "get|get_deleter ( )");
+    } else if (Token::Match(ftok->previous(), ". %name% (") && astIsContainer(ftok->previous()->astOperand1())) {
+        const Library::Container* container = ftok->previous()->astOperand1()->valueType()->container;
+        if (!container)
+            return false;
+        if (container->getYield(ftok->str()) != Library::Container::Yield::NO_YIELD)
+            return true;
+        if (container->getAction(ftok->str()) == Library::Container::Action::FIND)
+            return true;
+        return false;
     } else {
         bool memberFunction = Token::Match(ftok->previous(), ". %name% (");
         bool constMember = !memberFunction;

lib/symboldatabase.cpp

@@ -1418,7 +1418,7 @@ void SymbolDatabase::createSymbolDatabaseEscapeFunctions()
 
 static bool isExpression(const Token* tok)
 {
-    if (!Token::Match(tok, "(|.|[|%cop%"))
+    if (!Token::Match(tok, "(|.|[|::|?|:|++|--|%cop%|%assign%"))
         return false;
     if (Token::Match(tok, "*|&|&&")) {
         const Token* vartok = findAstNode(tok, [&](const Token* tok2) {
@@ -1444,6 +1444,7 @@ void SymbolDatabase::createSymbolDatabaseExprIds()
     }
     nonneg int id = base + 1;
     for (const Scope * scope : functionScopes) {
+        nonneg int thisId = 0;
         std::unordered_map<std::string, std::vector<Token*>> exprs;
 
         // Assign IDs
@@ -1457,6 +1458,10 @@ void SymbolDatabase::createSymbolDatabaseExprIds()
                 if (id == std::numeric_limits<nonneg int>::max()) {
                     throw InternalError(nullptr, "Ran out of expression ids.", InternalError::INTERNAL);
                 }
+            } else if (isCPP() && Token::simpleMatch(tok, "this")) {
+                if (thisId == 0)
+                    thisId = id++;
+                tok->exprId(thisId);
             }
         }
 

lib/valueflow.cpp

@@ -2191,6 +2191,8 @@ struct ValueFlowAnalyzer : Analyzer {
     {
         if (!useSymbolicValues())
             return false;
+        if (Token::Match(tok, "%assign%"))
+            return false;
         for (const ValueFlow::Value& v : tok->values()) {
             if (!v.isSymbolicValue())
                 continue;
@@ -2209,7 +2211,8 @@ struct ValueFlowAnalyzer : Analyzer {
 
     Action analyzeMatch(const Token* tok, Direction d) const {
         const Token* parent = tok->astParent();
-        if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0)
+        if ((astIsPointer(tok) || astIsSmartPointer(tok)) &&
+            (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0)
             return Action::Read;
 
         Action w = isWritable(tok, d);
@@ -2497,6 +2500,8 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer {
             return false;
         if (isConditional() && !value.isKnown() && !value.isImpossible())
             return true;
+        if (value.isSymbolicValue())
+            return false;
         ConditionState cs = analyzeCondition(condTok);
         return cs.isUnknownDependent();
     }
@@ -2563,14 +2568,18 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer {
         if (depth > maxDepth)
             return;
         visitAstNodes(start, [&](const Token* tok) {
-            for (const ValueFlow::Value& v : tok->values()) {
+            const bool top = depth == 0 && tok == start;
-                if (!(v.isLocalLifetimeValue() || (astIsPointer(tok) && v.isSymbolicValue() && v.isKnown())))
+            const bool ispointer = astIsPointer(tok) || astIsSmartPointer(tok);
-                    continue;
+            if (!top || !ispointer || value.indirect != 0) {
-                if (!v.tokvalue)
+                for (const ValueFlow::Value& v : tok->values()) {
-                    continue;
+                    if (!(v.isLocalLifetimeValue() || (ispointer && v.isSymbolicValue() && v.isKnown())))
-                if (v.tokvalue == tok)
+                        continue;
-                    continue;
+                    if (!v.tokvalue)
-                setupExprVarIds(v.tokvalue, depth + 1);
+                        continue;
+                    if (v.tokvalue == tok)
+                        continue;
+                    setupExprVarIds(v.tokvalue, depth + 1);
+                }
             }
             if (depth == 0 && tok->varId() == 0 && !tok->function() && tok->isName() && tok->previous()->str() != ".") {
                 // unknown variable
@@ -4312,6 +4321,7 @@ static bool isTruncated(const ValueType* src, const ValueType* dst, const Settin
 
 static void setSymbolic(ValueFlow::Value& value, const Token* tok)
 {
+    assert(tok && tok->exprId() > 0 && "Missing expr id for symbolic value");
     value.valueType = ValueFlow::Value::ValueType::SYMBOLIC;
     value.tokvalue = tok;
 }
@@ -4973,6 +4983,22 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
             const bool init = vars.size() == 1 && vars.front()->nameToken() == tok->astOperand1();
             valueFlowForwardAssign(
                 tok->astOperand2(), tok->astOperand1(), vars, values, init, tokenlist, errorLogger, settings);
+            // Back propagate symbolic values
+            if (tok->astOperand1()->exprId() > 0) {
+                Token* start = nextAfterAstRightmostLeaf(tok);
+                const Token* end = scope->bodyEnd;
+                for (ValueFlow::Value value : values) {
+                    if (!value.isSymbolicValue())
+                        continue;
+                    const Token* expr = value.tokvalue;
+                    value.intvalue = -value.intvalue;
+                    value.tokvalue = tok->astOperand1();
+                    value.errorPath.emplace_back(tok,
+                                                 tok->astOperand1()->expressionString() + " is assigned '" +
+                                                 tok->astOperand2()->expressionString() + "' here.");
+                    valueFlowForward(start, end, expr, {value}, tokenlist, settings);
+                }
+            }
         }
     }
 }
@@ -5690,6 +5716,8 @@ struct SymbolicConditionHandler : SimpleConditionHandler {
             const bool lhs = i == 0;
             const Token* vartok = lhs ? tok->astOperand1() : tok->astOperand2();
             const Token* valuetok = lhs ? tok->astOperand2() : tok->astOperand1();
+            if (valuetok->exprId() == 0)
+                continue;
             if (valuetok->hasKnownSymbolicValue(vartok))
                 continue;
             if (vartok->hasKnownSymbolicValue(valuetok))
@@ -6842,6 +6870,17 @@ static bool isContainerSizeChanged(nonneg int varId,
     return false;
 }
 
+std::vector<const Variable*> getVariables(const Token* tok)
+{
+    std::vector<const Variable*> result;
+    visitAstNodes(tok, [&](const Token* child) {
+        if (child->variable())
+            result.push_back(child->variable());
+        return ChildrenToVisit::op1_and_op2;
+    });
+    return result;
+}
+
 static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogger, const Settings *settings)
 {
     for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
@@ -6849,7 +6888,9 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge
             continue;
         if (!tok->scope()->isExecutable())
             continue;
-        if (tok->variable()) {
+        if (!astIsSmartPointer(tok))
+            continue;
+        if (tok->variable() && Token::Match(tok, "%var% (|{|;")) {
             const Variable* var = tok->variable();
             if (!var->isSmartPointer())
                 continue;
@@ -6868,27 +6909,32 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge
                     values.push_back(v);
                     valueFlowForwardAssign(tok, var, values, false, true, tokenlist, errorLogger, settings);
                 }
-            } else if (Token::Match(tok, "%var% . reset (") && tok->next()->originalName() != "->") {
+            }
-                if (Token::simpleMatch(tok->tokAt(3), "( )")) {
+        } else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (") &&
+                   tok->astParent()->originalName() != "->") {
+            std::vector<const Variable*> vars = getVariables(tok);
+            Token* ftok = tok->astParent()->tokAt(2);
+            if (Token::simpleMatch(tok->astParent(), ". reset (")) {
+                if (Token::simpleMatch(ftok, "( )")) {
                     std::list<ValueFlow::Value> values;
                     ValueFlow::Value v(0);
                     v.setKnown();
                     values.push_back(v);
-                    valueFlowForwardAssign(tok->tokAt(3), var, values, false, false, tokenlist, errorLogger, settings);
+                    valueFlowForwardAssign(ftok, tok, vars, values, false, tokenlist, errorLogger, settings);
                 } else {
                     tok->removeValues(std::mem_fn(&ValueFlow::Value::isIntValue));
-                    Token* inTok = tok->tokAt(3)->astOperand2();
+                    Token* inTok = ftok->astOperand2();
                     if (!inTok)
                         continue;
                     std::list<ValueFlow::Value> values = inTok->values();
-                    const bool constValue = inTok->isNumber();
+                    valueFlowForwardAssign(inTok, tok, vars, values, false, tokenlist, errorLogger, settings);
-                    valueFlowForwardAssign(inTok, var, values, constValue, false, tokenlist, errorLogger, settings);
                 }
-            } else if (Token::Match(tok, "%var% . release ( )") && tok->next()->originalName() != "->") {
+            } else if (Token::simpleMatch(tok->astParent(), ". release ( )")) {
-                const Token* parent = tok->tokAt(3)->astParent();
+                const Token* parent = ftok->astParent();
                 bool hasParentReset = false;
                 while (parent) {
-                    if (Token::Match(parent->tokAt(-3), "%varid% . release|reset (", tok->varId())) {
+                    if (Token::Match(parent->tokAt(-2), ". release|reset (") &&
+                        parent->tokAt(-2)->astOperand1()->exprId() == tok->exprId()) {
                         hasParentReset = true;
                         break;
                     }
@@ -6900,7 +6946,10 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge
                 ValueFlow::Value v(0);
                 v.setKnown();
                 values.push_back(v);
-                valueFlowForwardAssign(tok->tokAt(3), var, values, false, false, tokenlist, errorLogger, settings);
+                valueFlowForwardAssign(ftok, tok, vars, values, false, tokenlist, errorLogger, settings);
+            } else if (Token::simpleMatch(tok->astParent(), ". get ( )")) {
+                ValueFlow::Value v = makeSymbolic(tok);
+                setTokenValue(tok->astParent()->tokAt(2), v, settings);
             }
         } else if (Token::Match(tok->previous(), "%name%|> (|{") && astIsSmartPointer(tok) &&
                    astIsSmartPointer(tok->astOperand1())) {

test/testcondition.cpp

@@ -1573,7 +1573,7 @@ private:
               "        ;\n"
               "    }\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:200] -> [test.cpp:200]: (style) Condition 'g' is always true\n", errout.str());
     }
 
     void incorrectLogicOperator15() {

test/testnullpointer.cpp

@@ -123,6 +123,7 @@ private:
         TEST_CASE(nullpointer81); // #8724
         TEST_CASE(nullpointer82); // #10331
         TEST_CASE(nullpointer83); // #9870
+        TEST_CASE(nullpointer84); // #9873
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2511,6 +2512,19 @@ private:
         ASSERT_EQUALS("[test.cpp:8]: (warning) Possible null pointer dereference: p\n", errout.str());
     }
 
+    void nullpointer84() // #9873
+    {
+        check("void f(std::unique_ptr<A> P) {\n"
+              "  A *RP = P.get();\n"
+              "  if (!RP) {\n"
+              "    P->foo();\n"
+              "  }\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition '!RP' is redundant or there is possible null pointer dereference: P.\n",
+            errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"

test/testvarid.cpp

@@ -3423,9 +3423,9 @@ private:
                                 "2: int x ; int y ;\n"
                                 "3: } ;\n"
                                 "4: int f ( A a , A b ) {\n"
-                                "5: int x@5 ; x@5 = a@3 .@9 x@6 +@10 b@4 .@11 x@7 ;\n"
+                                "5: int x@5 ; x@5 =@9 a@3 .@10 x@6 +@11 b@4 .@12 x@7 ;\n"
-                                "6: int y@8 ; y@8 = b@4 .@11 x@7 +@10 a@3 .@9 x@6 ;\n"
+                                "6: int y@8 ; y@8 =@13 b@4 .@12 x@7 +@11 a@3 .@10 x@6 ;\n"
-                                "7: return x@5 +@15 y@8 +@16 a@3 .@17 y@9 +@18 b@4 .@19 y@10 ;\n"
+                                "7: return x@5 +@17 y@8 +@18 a@3 .@19 y@9 +@20 b@4 .@21 y@10 ;\n"
                                 "8: }\n";
 
         ASSERT_EQUALS(expected, actual);