Improve diagnostics with null smart pointers (#1805)

* Warn when dereferencing null smart pointers

* Improve tracking of smart pointer values

* Use library isSmartPointer

lib/checknullpointer.cpp

@@ -329,7 +329,10 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
         }
 
         const Variable *var = tok->variable();
-        if (!var || !var->isPointer() || tok == var->nameToken())
+        if (!var || tok == var->nameToken())
+            continue;
+
+        if (!var->isPointer() && !var->isSmartPointer())
             continue;
 
         // Can pointer be NULL?

lib/symboldatabase.cpp

@@ -1659,6 +1659,7 @@ void Variable::evaluate(const Settings* settings)
         setFlag(fIsClass, !lib->podtype(strtype) && !mTypeStartToken->isStandardType() && !isEnumType() && !isPointer() && !isReference());
         setFlag(fIsStlType, Token::simpleMatch(mTypeStartToken, "std ::"));
         setFlag(fIsStlString, isStlType() && (Token::Match(mTypeStartToken->tokAt(2), "string|wstring|u16string|u32string !!::") || (Token::simpleMatch(mTypeStartToken->tokAt(2), "basic_string <") && !Token::simpleMatch(mTypeStartToken->linkAt(3), "> ::"))));
+        setFlag(fIsSmartPointer, lib->isSmartPointer(mTypeStartToken));
     }
     if (mAccess == Argument) {
         tok = mNameToken;

lib/symboldatabase.h

@@ -190,7 +190,8 @@ class CPPCHECKLIB Variable {
         fIsStlType    = (1 << 10),  /** @brief STL type ('std::') */
         fIsStlString  = (1 << 11),  /** @brief std::string|wstring|basic_string&lt;T&gt;|u16string|u32string */
         fIsFloatType  = (1 << 12),  /** @brief Floating point type */
-        fIsVolatile  = (1 << 13)  /** @brief volatile */
+        fIsVolatile   = (1 << 13),  /** @brief volatile */
+        fIsSmartPointer = (1 << 14)   /** @brief std::shared_ptr|unique_ptr */
     };
 
     /**
@@ -555,6 +556,10 @@ public:
         return getFlag(fIsStlString);
     }
 
+    bool isSmartPointer() const {
+        return getFlag(fIsSmartPointer);
+    }
+
     /**
      * Checks if the variable is of any of the STL types passed as arguments ('std::')
      * E.g.:

lib/valueflow.cpp

@@ -3496,6 +3496,67 @@ static void valueFlowAfterMove(TokenList *tokenlist, SymbolDatabase* symboldatab
     }
 }
 
+static void valueFlowForwardAssign(Token * const               tok,
+                             const Variable * const      var,
+                             std::list<ValueFlow::Value> values,
+                             const bool                  constValue,
+                             const bool                  init,
+                             TokenList * const           tokenlist,
+                             ErrorLogger * const         errorLogger,
+                             const Settings * const      settings)
+{
+    const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
+    if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) {
+        valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
+        values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
+    }
+    if (!var->isPointer() && !var->isSmartPointer())
+        values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
+    if (tok->astParent()) {
+        for (std::list<ValueFlow::Value>::iterator it = values.begin(); it != values.end(); ++it) {
+            const std::string info = "Assignment '" + tok->astParent()->expressionString() + "', assigned value is " + it->infoString();
+            it->errorPath.emplace_back(tok, info);
+        }        
+    }
+
+    if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) {
+        std::list<ValueFlow::Value>::iterator it;
+        for (it = values.begin(); it != values.end(); ++it) {
+            if (it->isIntValue())
+                it->intvalue = (it->intvalue != 0);
+            if (it->isTokValue())
+                it ->intvalue = (it->tokvalue != 0);
+        }
+    }
+
+    // Static variable initialisation?
+    if (var->isStatic() && init)
+        changeKnownToPossible(values);
+
+    // Skip RHS
+    const Token * nextExpression = tok->astParent() ? nextAfterAstRightmostLeaf(tok->astParent()) : tok->next();
+
+    if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isTokValue))) {
+        std::list<ValueFlow::Value> tokvalues;
+        std::copy_if(values.begin(),
+                     values.end(),
+                     std::back_inserter(tokvalues),
+                     std::mem_fn(&ValueFlow::Value::isTokValue));
+        valueFlowForward(const_cast<Token *>(nextExpression),
+                         endOfVarScope,
+                         var,
+                         var->declarationId(),
+                         tokvalues,
+                         constValue,
+                         false,
+                         tokenlist,
+                         errorLogger,
+                         settings);
+        values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
+    }
+    valueFlowForward(const_cast<Token *>(nextExpression), endOfVarScope, var, var->declarationId(), values, constValue, false, tokenlist, errorLogger, settings);
+}
+
 static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
 {
     for (const Scope * scope : symboldatabase->functionScopes) {
@@ -3521,61 +3582,14 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
             if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument()))
                 continue;
 
-            const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
-
             // Rhs values..
             if (!tok->astOperand2() || tok->astOperand2()->values().empty())
                 continue;
 
             std::list<ValueFlow::Value> values = tok->astOperand2()->values();
-            if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) {
-                valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
-                values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
-            }
-            if (!var->isPointer())
-                values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
-            for (std::list<ValueFlow::Value>::iterator it = values.begin(); it != values.end(); ++it) {
-                const std::string info = "Assignment '" + tok->expressionString() + "', assigned value is " + it->infoString();
-                it->errorPath.emplace_back(tok->astOperand2(), info);
-            }
             const bool constValue = tok->astOperand2()->isNumber();
-
+            const bool init = var->nameToken() == tok->astOperand1();
-            if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) {
+            valueFlowForwardAssign(const_cast<Token *>(tok->astOperand2()), var, values, constValue, init, tokenlist, errorLogger, settings);
-                std::list<ValueFlow::Value>::iterator it;
-                for (it = values.begin(); it != values.end(); ++it) {
-                    if (it->isIntValue())
-                        it->intvalue = (it->intvalue != 0);
-                    if (it->isTokValue())
-                        it ->intvalue = (it->tokvalue != 0);
-                }
-            }
-
-            // Static variable initialisation?
-            if (var->isStatic() && var->nameToken() == tok->astOperand1())
-                changeKnownToPossible(values);
-
-            // Skip RHS
-            const Token * nextExpression = nextAfterAstRightmostLeaf(tok);
-
-            if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isTokValue))) {
-                std::list<ValueFlow::Value> tokvalues;
-                std::copy_if(values.begin(),
-                             values.end(),
-                             std::back_inserter(tokvalues),
-                             std::mem_fn(&ValueFlow::Value::isTokValue));
-                valueFlowForward(const_cast<Token *>(nextExpression),
-                                 endOfVarScope,
-                                 var,
-                                 varid,
-                                 tokvalues,
-                                 constValue,
-                                 false,
-                                 tokenlist,
-                                 errorLogger,
-                                 settings);
-                values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
-            }
-            valueFlowForward(const_cast<Token *>(nextExpression), endOfVarScope, var, varid, values, constValue, false, tokenlist, errorLogger, settings);
         }
     }
 }
@@ -4954,6 +4968,55 @@ static bool isContainerSizeChanged(unsigned int varId, const Token *start, const
     return false;
 }
 
+static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogger, const Settings *settings)
+{
+    for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
+        if (!tok->scope())
+            continue;
+        if (!tok->scope()->isExecutable())
+            continue;
+        if (!tok->variable())
+            continue;
+        const Variable * var = tok->variable();
+        if (!var->isSmartPointer())
+            continue;
+        if (var->nameToken() == tok) {
+            if (Token::Match(tok, "%var% (|{") && tok->next()->astOperand2() && tok->next()->astOperand2()->str() != ",") {
+                const Token * inTok = tok->next()->astOperand2();
+                std::list<ValueFlow::Value> values = inTok->values();
+                const bool constValue = inTok->isNumber();
+                valueFlowForwardAssign(const_cast<Token *>(inTok), var, values, constValue, true, tokenlist, errorLogger, settings);
+
+            } else if (Token::Match(tok, "%var% ;")) {
+                std::list<ValueFlow::Value> values;
+                ValueFlow::Value v(0);
+                v.setKnown();
+                values.push_back(v);
+                valueFlowForwardAssign(tok, var, values, false, true, tokenlist, errorLogger, settings);
+            }
+        } else if (Token::Match(tok, "%var% . reset (")) {
+            if (Token::simpleMatch(tok->tokAt(3), "( )")) {
+                std::list<ValueFlow::Value> values;
+                ValueFlow::Value v(0);
+                v.setKnown();
+                values.push_back(v);
+                valueFlowForwardAssign(tok->tokAt(4), var, values, false, false, tokenlist, errorLogger, settings);
+            } else {
+                const Token * inTok = tok->tokAt(3)->astOperand2();
+                std::list<ValueFlow::Value> values = inTok->values();
+                const bool constValue = inTok->isNumber();
+                valueFlowForwardAssign(const_cast<Token *>(inTok), var, values, constValue, false, tokenlist, errorLogger, settings);
+            }
+        } else if (Token::Match(tok, "%var% . release ( )")) {
+            std::list<ValueFlow::Value> values;
+            ValueFlow::Value v(0);
+            v.setKnown();
+            values.push_back(v);
+            valueFlowForwardAssign(tok->tokAt(4), var, values, false, false, tokenlist, errorLogger, settings);
+        }
+    }   
+}
+
 static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings)
 {
     // declaration
@@ -5302,6 +5365,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
         valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings);
         if (tokenlist->isCPP()) {
+            valueFlowSmartPointer(tokenlist, errorLogger, settings);
             valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings);
             valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
         }

test/testnullpointer.cpp

@@ -90,6 +90,7 @@ private:
         TEST_CASE(nullpointerExit);
         TEST_CASE(nullpointerStdString);
         TEST_CASE(nullpointerStdStream);
+        TEST_CASE(nullpointerSmartPointer);
         TEST_CASE(functioncall);
         TEST_CASE(functioncalllibrary); // use Library to parse function call
         TEST_CASE(functioncallDefaultArguments);
@@ -2259,6 +2260,80 @@ private:
 
     }
 
+    void nullpointerSmartPointer() {
+        check("struct Fred { int x; };\n"
+              "void f(std::shared_ptr<Fred> p) {\n"
+              "  if (p) {}\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p.\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::shared_ptr<Fred> p) {\n"
+              "  p = nullptr;\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::unique_ptr<Fred> p) {\n"
+              "  if (p) {}\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p.\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::unique_ptr<Fred> p) {\n"
+              "  p = nullptr;\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f() {\n"
+              "  std::shared_ptr<Fred> p;\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::shared_ptr<Fred> p) {\n"
+              "  p.reset();\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::shared_ptr<Fred> p) {\n"
+              "  Fred * pp = nullptr;\n"
+              "  p.reset(pp);\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(Fred& f) {\n"
+              "  std::shared_ptr<Fred> p;\n"
+              "  p.reset(&f);\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f(std::shared_ptr<Fred> p) {\n"
+              "  p.release();\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+
+        check("struct Fred { int x; };\n"
+              "void f() {\n"
+              "  std::shared_ptr<Fred> p(nullptr);\n"
+              "  dostuff(p->x);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str());
+    }
+
     void functioncall() {    // #3443 - function calls
         // dereference pointer and then check if it's null
         {