Improved nullpointer check:

- More accurate checking for dereferences and non-dereferences
- improved checking for nullpointer dereferences after return statement
- Supports pointer dereferences by std::string
- Code optimization/refactorization

lib/checknullpointer.cpp

@@ -129,7 +129,7 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::list<const Token
     }
 
     // 2nd parameter..
-    if (Token::Match(&tok, "%var% ( %any%")) {
+    if (Token::Match(&tok, "%var% ( !!)")) {
         const Token* secondParameter = tok.tokAt(2)->nextArgument();
         if (secondParameter && ((value == 0 && secondParameter->str() == "0") || (Token::Match(secondParameter, "%var%") && secondParameter->varId() > 0))) {
             if (functionNames2_all.find(tok.str()) != functionNames2_all.end())
@@ -216,14 +216,14 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::list<const Token
  * @param unknown it is not known if there is a pointer dereference (could be reported as a debug message)
  * @return true => there is a dereference
  */
-bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown)
+bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const SymbolDatabase* symbolDatabase)
 {
     const bool inconclusive = unknown;
 
     unknown = false;
 
     // Dereferencing pointer..
-    if (Token::Match(tok->tokAt(-3), "!!sizeof [;{}=+-/(,] * %var%") && Token::Match(tok->tokAt(-3), "!!decltype [;{}=+-/(,] * %var%"))
+    if (tok->strAt(-1) == "*" && (Token::Match(tok->tokAt(-2), "return|throw|;|{|}|:|[|(|,") || tok->tokAt(-2)->isOp() || tok->tokAt(-2)->isAssignmentOp()) && !Token::Match(tok->tokAt(-3), "sizeof|decltype"))
         return true;
 
     // read/write member variable
@@ -234,7 +234,7 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown)
         return false;
     }
 
-    if (Token::Match(tok->previous(), "!!& %var% ["))
+    if (Token::Match(tok, "%var% [") && (tok->previous()->str() != "&" || Token::Match(tok->next()->link()->next(), "[.(]")))
         return true;
 
     if (Token::Match(tok, "%var% ("))
@@ -245,6 +245,23 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown)
         tok->varId() == tok->tokAt(2)->varId())
         return true;
 
+    // std::string dereferences nullpointers
+    if (Token::Match(tok->tokAt(-4), "std :: string ( %var% )"))
+        return true;
+    if (Token::Match(tok->tokAt(-5), "std :: string %var% ( %var% )"))
+        return true;
+
+    unsigned int ovarid = 0;
+    if (Token::Match(tok, "%var% ==|!= %var%"))
+        ovarid = tok->tokAt(2)->varId();
+    else if (Token::Match(tok->tokAt(-2), "%var% ==|!=|=|+= %var%"))
+        ovarid = tok->tokAt(-2)->varId();
+    if (ovarid) {
+        const Variable* var = symbolDatabase->getVariableFromVarId(ovarid);
+        if (var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "const| std :: string !!::"))
+            return true;
+    }
+
     // Check if it's NOT a pointer dereference.
     // This is most useful in inconclusive checking
     if (inconclusive) {
@@ -382,7 +399,7 @@ void CheckNullPointer::nullPointerAfterLoop()
                 bool unknown = _settings->inconclusive;
 
                 // Is the loop variable dereferenced?
-                if (CheckNullPointer::isPointerDeRef(tok2, unknown)) {
+                if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase)) {
                     nullPointerError(tok2, varname, tok->linenr(), inconclusive);
                 }
 
@@ -422,31 +439,33 @@ void CheckNullPointer::nullPointerLinkedList()
                 if (varid == 0)
                     continue;
 
+                // Is this variable a pointer?
+                const Variable* var = symbolDatabase->getVariableFromVarId(varid);
+                if (!var || !var->isPointer())
+                    continue;
+
                 if (Token::Match(tok2->tokAt(-2), "%varid% ?", varid))
                     continue;
 
-                // Variable name of dereferenced variable
-                const std::string varname(tok2->str());
-
                 // Check usage of dereferenced variable in the loop..
-                for (const Token *tok3 = i->classStart->next(); tok3 && tok3 != i->classEnd; tok3 = tok3->next()) {
+                for (std::list<Scope*>::const_iterator j = i->nestedList.begin(); j != i->nestedList.end(); ++j) {
+                    Scope* scope = *j;
+                    if (scope->type != Scope::eWhile)
+                        continue;
+
                     // TODO: are there false negatives for "while ( %varid% ||"
-                    if (Token::Match(tok3, "while ( %varid% &&|)", varid)) {
+                    if (Token::Match(scope->classDef->next(), "( %varid% &&|)", varid)) {
                         // Make sure there is a "break" or "return" inside the loop.
                         // Without the "break" a null pointer could be dereferenced in the
                         // for statement.
                         // indentlevel4 is a counter for { and }. When scanning the code with tok4
                         unsigned int indentlevel4 = 1;
-                        for (const Token *tok4 = tok3->next()->link(); tok4; tok4 = tok4->next()) {
+                        for (const Token *tok4 = scope->classStart; tok4; tok4 = tok4->next()) {
                             if (tok4->str() == "{")
                                 ++indentlevel4;
                             else if (tok4->str() == "}") {
                                 if (indentlevel4 <= 1) {
-                                    const Variable* var = symbolDatabase->getVariableFromVarId(varid);
+                                    nullPointerError(tok1, var->name(), scope->classDef->linenr());
-                                    // Is this variable a pointer?
-                                    if (var && var->isPointer())
-                                        nullPointerError(tok1, varname, tok3->linenr());
-
                                     break;
                                 }
                                 --indentlevel4;
@@ -758,7 +777,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
 
                     // unknown : this is set by isPointerDeRef if it is
                     //           uncertain
-                    bool unknown = false;
+                    bool unknown = _settings->inconclusive;
 
                     if (Token::Match(tok1->tokAt(-2), "%varid% = %varid% .", varid)) {
                         break;
@@ -772,7 +791,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
                         break;
                     } else if (Token::Match(tok1->tokAt(-2), "&&|%oror% !")) {
                         break;
-                    } else if (CheckNullPointer::isPointerDeRef(tok1, unknown)) {
+                    } else if (CheckNullPointer::isPointerDeRef(tok1, unknown, symbolDatabase)) {
                         nullPointerError(tok1, varname, tok->linenr(), inconclusive);
                         break;
                     } else if (Token::simpleMatch(tok1->previous(), "&")) {
@@ -891,16 +910,22 @@ void CheckNullPointer::nullPointerByCheckAndDeRef()
                 }
             }
 
-            if (Token::Match(tok2, "goto|return|continue|break|throw|if|switch|for")) {
+            if (tok2->str() == "return" || tok2->str() == "throw") {
-                bool dummy = false;
+                bool unknown = _settings->inconclusive;
-                if (Token::Match(tok2, "return * %varid%", varid))
+                for (; tok2 && tok2->str() != ";"; tok2 = tok2->next()) {
-                    nullPointerError(tok2, pointerName, linenr, inconclusive);
+                    if (tok2->varId() == varid) {
-                else if (Token::Match(tok2, "return %varid%", varid) &&
+                        if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase))
-                         CheckNullPointer::isPointerDeRef(tok2->next(), dummy))
+                            nullPointerError(tok2, pointerName, linenr, inconclusive);
-                    nullPointerError(tok2, pointerName, linenr, inconclusive);
+                        else if (unknown)
+                            nullPointerError(tok2, pointerName, linenr, true);
+                    }
+                }
                 break;
             }
 
+            if (Token::Match(tok2, "goto|continue|break|if|switch|for"))
+                break;
+
             // parameters to sizeof are not dereferenced
             if (Token::Match(tok2, "decltype|sizeof")) {
                 if (tok2->strAt(1) != "(")
@@ -949,7 +974,7 @@ void CheckNullPointer::nullPointerByCheckAndDeRef()
                 if (Token::Match(tok2->previous(), "[;{}=] %var% = 0 ;"))
                     ;
 
-                else if (CheckNullPointer::isPointerDeRef(tok2, unknown))
+                else if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase))
                     nullPointerError(tok2, pointerName, linenr, inconclusive);
 
                 else if (unknown && _settings->inconclusive)
@@ -982,15 +1007,18 @@ void CheckNullPointer::nullConstantDereference()
             continue;
 
         for (const Token *tok = i->classStart; tok != i->classEnd; tok = tok->next()) {
-            if (tok->str() == "(" && Token::Match(tok->previous(), "sizeof|decltype|typeid"))
+            if (Token::Match(tok, "sizeof|decltype|typeid ("))
-                tok = tok->link();
+                tok = tok->next()->link();
 
             else if (Token::simpleMatch(tok, "* 0")) {
-                if (Token::Match(tok->previous(), "return|;|{|}|=|(|,|%op%")) {
+                if (Token::Match(tok->previous(), "return|throw|;|{|}|:|[|(|,") || tok->previous()->isOp() || tok->previous()->isAssignmentOp()) {
                     nullPointerError(tok);
                 }
             }
 
+            else if (Token::Match(tok, "0 [") && (tok->previous()->str() != "&" || !Token::Match(tok->next()->link()->next(), "[.(]")))
+                nullPointerError(tok);
+
             else if (Token::Match(tok->previous(), "[={};] %var% (")) {
                 std::list<const Token *> var;
                 parseFunctionCall(*tok, var, 0);
@@ -1001,6 +1029,20 @@ void CheckNullPointer::nullConstantDereference()
                         nullPointerError(*it);
                     }
                 }
+            } else if (Token::Match(tok, "std :: string ( 0 )"))
+                nullPointerError(tok);
+            if (Token::Match(tok, "std :: string %var% ( 0 )"))
+                nullPointerError(tok);
+
+            unsigned int ovarid = 0;
+            if (Token::Match(tok, "0 ==|!= %var%"))
+                ovarid = tok->tokAt(2)->varId();
+            else if (Token::Match(tok, "%var% ==|!=|=|+= 0"))
+                ovarid = tok->varId();
+            if (ovarid) {
+                const Variable* var = symbolDatabase->getVariableFromVarId(ovarid);
+                if (var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "const| std :: string !!::"))
+                    nullPointerError(tok);
             }
         }
     }
@@ -1018,13 +1060,16 @@ void CheckNullPointer::nullConstantDereference()
 class Nullpointer : public ExecutionPath {
 public:
     /** Startup constructor */
-    explicit Nullpointer(Check *c) : ExecutionPath(c, 0), null(false) {
+    Nullpointer(Check *c, const SymbolDatabase* symbolDatabase_) : ExecutionPath(c, 0), symbolDatabase(symbolDatabase_), null(false) {
     }
 
 private:
+    const SymbolDatabase* symbolDatabase;
+
     /** Create checking of specific variable: */
-    Nullpointer(Check *c, const unsigned int id, const std::string &name)
+    Nullpointer(Check *c, const unsigned int id, const std::string &name, const SymbolDatabase* symbolDatabase_)
         : ExecutionPath(c, id),
+          symbolDatabase(symbolDatabase_),
           varname(name),
           null(false) {
     }
@@ -1082,56 +1127,22 @@ private:
 
     /** parse tokens */
     const Token *parse(const Token &tok, std::list<ExecutionPath *> &checks) const {
-        if (Token::Match(tok.previous(), "[;{}] const| struct| %type% * %var% ;")) {
+        if (tok.varId() != 0) {
-            const Token * vartok = tok.tokAt(2);
+            // Pointer declaration declaration?
-
+            const Variable* var = symbolDatabase->getVariableFromVarId(tok.varId());
-            if (tok.str() == "const")
+            if (var && var->isPointer() && var->nameToken() == &tok)
-                vartok = vartok->next();
+                checks.push_back(new Nullpointer(owner, var->varId(), var->name(), symbolDatabase));
-
-            if (tok.str() == "struct")
-                vartok = vartok->next();
-
-            if (vartok->varId() != 0)
-                checks.push_back(new Nullpointer(owner, vartok->varId(), vartok->str()));
-            return vartok->next();
-        }
-
-        // Template pointer variable..
-        if (Token::Match(tok.previous(), "[;{}] %type% ::|<")) {
-            const Token * vartok = &tok;
-            while (Token::Match(vartok, "%type% ::"))
-                vartok = vartok->tokAt(2);
-            if (Token::Match(vartok, "%type% < %type%")) {
-                vartok = vartok->tokAt(3);
-                while (vartok && (vartok->str() == "*" || vartok->isName()))
-                    vartok = vartok->next();
-            }
-            if (vartok
-                && (vartok->str() == ">" || vartok->isName())
-                && Token::Match(vartok->next(), "* %var% ;|=")) {
-                vartok = vartok->tokAt(2);
-                checks.push_back(new Nullpointer(owner, vartok->varId(), vartok->str()));
-                if (Token::simpleMatch(vartok->next(), "= 0 ;"))
-                    setnull(checks, vartok->varId());
-                return vartok->next();
-            }
         }
 
         if (Token::simpleMatch(&tok, "try {")) {
             // Bail out all used variables
-            unsigned int indentlevel = 0;
+            const Token* tok2 = &tok;
-            for (const Token *tok2 = &tok; tok2; tok2 = tok2->next()) {
+            const Token* end = tok.linkAt(1);
-                if (tok2->str() == "{")
+            for (; tok2 && tok2 != end; tok2 = tok2->next()) {
-                    ++indentlevel;
+                if (tok2->varId())
-                else if (tok2->str() == "}") {
-                    if (indentlevel == 0)
-                        break;
-                    if (indentlevel == 1 && !Token::simpleMatch(tok2,"} catch ("))
-                        return tok2;
-                    --indentlevel;
-                } else if (tok2->varId())
                     bailOutVar(checks,tok2->varId());
             }
+            return tok2;
         }
 
         if (Token::Match(&tok, "%var% (")) {
@@ -1149,21 +1160,21 @@ private:
             return tok.link();
 
         if (tok.varId() != 0) {
-            // unknown : not really used. it is passed to isPointerDeRef.
+            // unknown: if isPointerDeRef fails to determine if there
-            //           if isPointerDeRef fails to determine if there
+            //          is a dereference this will be set to true.
-            //           is a dereference the this will be set to true.
             bool unknown = owner->inconclusiveFlag();
+            bool deref = CheckNullPointer::isPointerDeRef(&tok, unknown, symbolDatabase);
 
-            if (Token::Match(tok.previous(), "[;{}=] %var% = 0 ;"))
+            if (deref)
-                setnull(checks, tok.varId());
-            else if (CheckNullPointer::isPointerDeRef(&tok, unknown))
                 dereference(checks, &tok);
             else if (unknown && owner->inconclusiveFlag())
                 dereference(checks, &tok);
-            else
+            if (Token::Match(tok.previous(), "[;{}=] %var% = 0 ;"))
-                // TODO: Report debug warning that it's unknown if a
+                setnull(checks, tok.varId());
-                // pointer is dereferenced
+            else if (!deref &&
-                bailOutVar(checks, tok.varId());
+                     !tok.previous()->isOp() && !tok.previous()->isAssignmentOp() &&
+                     (!tok.next()->isOp() || tok.next()->str() == ">>"))
+                bailOutVar(checks, tok.varId()); // If its possible that the pointers value changes, bail out.
         }
 
         else if (tok.str() == "delete") {
@@ -1175,13 +1186,15 @@ private:
         }
 
         else if (tok.str() == "return") {
-            bool unknown = false;
+            bool unknown = owner->inconclusiveFlag();
-            const Token *vartok = tok.next();
+            const Token* tok2 = &tok;
-            if (vartok->str() == "*")
+            for (; tok2 && tok2->str() != ";"; tok2 = tok2->next()) {
-                vartok = vartok->next();
+                if (tok2->varId()) {
-            if (vartok->varId() && CheckNullPointer::isPointerDeRef(vartok, unknown)) {
+                    if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase) || unknown)
-                dereference(checks, vartok);
+                        dereference(checks, tok2);
+                }
             }
+            return tok2;
         }
 
         return &tok;
@@ -1192,8 +1205,9 @@ private:
         for (const Token *tok2 = &tok; tok2; tok2 = tok2->next()) {
             if (tok2->str() == "(" || tok2->str() == ")")
                 break;
-            if (Token::Match(tok2, "[<>=] * %var%"))
+            bool unknown = owner->inconclusiveFlag();
-                dereference(checks, tok2->tokAt(2));
+            if (tok2->varId() && (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase) || unknown))
+                dereference(checks, tok2);
         }
 
         if (Token::Match(&tok, "!| %var% (")) {
@@ -1224,7 +1238,7 @@ private:
 void CheckNullPointer::executionPaths()
 {
     // Check for null pointer errors..
-    Nullpointer c(this);
+    Nullpointer c(this, _tokenizer->getSymbolDatabase());
     checkExecutionPaths(_tokenizer->tokens(), &c);
 }
 
@@ -1246,6 +1260,3 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
     else
         reportError(tok, Severity::error, "nullPointer", errmsg);
 }
-
-
-

lib/checknullpointer.h

@@ -26,6 +26,7 @@
 #include "settings.h"
 
 class Token;
+class SymbolDatabase;
 
 /// @addtogroup Checks
 /// @{
@@ -80,7 +81,7 @@ public:
      * @param unknown it is not known if there is a pointer dereference (could be reported as a debug message)
      * @return true => there is a dereference
      */
-    static bool isPointerDeRef(const Token *tok, bool &unknown);
+    static bool isPointerDeRef(const Token *tok, bool &unknown, const SymbolDatabase* symbolDatabase);
 
     /** @brief possible null pointer dereference */
     void nullPointer();

lib/checkuninitvar.cpp

@@ -1262,7 +1262,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer) const
     }
 
     bool unknown = false;
-    if (pointer && CheckNullPointer::isPointerDeRef(vartok, unknown)) {
+    if (pointer && CheckNullPointer::isPointerDeRef(vartok, unknown, _tokenizer->getSymbolDatabase())) {
         // function parameter?
         bool functionParameter = false;
         if (Token::Match(vartok->tokAt(-2), "%var% (") || vartok->previous()->str() == ",")

test/testnullpointer.cpp

@@ -60,6 +60,7 @@ private:
         TEST_CASE(nullpointer_in_for_loop);
         TEST_CASE(nullpointerDelete);
         TEST_CASE(nullpointerExit);
+        TEST_CASE(nullpointerStdString);
 
         TEST_CASE(functioncall);
     }
@@ -1283,6 +1284,15 @@ private:
               "}\n");
         ASSERT_EQUALS("", errout.str());
 
+        check("int foo(int *p) {\n"
+              "    if (!p) {\n"
+              "        x = *p;\n"
+              "        return 5+*p;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Possible null pointer dereference: p - otherwise it is redundant to check if p is null at line 2\n"
+                      "[test.cpp:4]: (error) Possible null pointer dereference: p - otherwise it is redundant to check if p is null at line 2\n", errout.str());
+
         // operator!
         check("void f() {\n"
               "    A a;\n"
@@ -1486,6 +1496,12 @@ private:
               "    image1.fseek(0, SEEK_SET);\n"
               "}");
         ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    int* p = 0;\n"
+              "    return p[4];\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n", errout.str());
     }
 
     void gcc_statement_expression() {
@@ -1719,6 +1735,42 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointerStdString() {
+        check("void f(std::string s1) {\n"
+              "    void* p = 0;\n"
+              "    s1 = 0;\n"
+              "    std::string s2 = 0;\n"
+              "    std::string s3(0);\n"
+              "    foo(std::string(0));\n"
+              "    s1 = p;\n"
+              "    std::string s4 = p;\n"
+              "    std::string s5(p);\n"
+              "    foo(std::string(p));\n"
+              "}", true);
+        ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n"
+                      "[test.cpp:4]: (error) Null pointer dereference\n"
+                      "[test.cpp:5]: (error) Null pointer dereference\n"
+                      "[test.cpp:6]: (error) Null pointer dereference\n"
+                      "[test.cpp:7]: (error) Possible null pointer dereference: p\n"
+                      "[test.cpp:8]: (error) Possible null pointer dereference: p\n"
+                      "[test.cpp:9]: (error) Possible null pointer dereference: p\n"
+                      "[test.cpp:10]: (error) Possible null pointer dereference: p\n", errout.str());
+
+        check("void f(std::string s1, const std::string& s2, const std::string* s3) {\n"
+              "    void* p = 0;\n"
+              "    foo(s1 == p);\n"
+              "    foo(s2 == p);\n"
+              "    foo(s3 == p);\n"
+              "    foo(p == s1);\n"
+              "    foo(p == s2);\n"
+              "    foo(p == s3);\n"
+              "}", true);
+        ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n"
+                      "[test.cpp:4]: (error) Possible null pointer dereference: p\n"
+                      "[test.cpp:6]: (error) Possible null pointer dereference: p\n"
+                      "[test.cpp:7]: (error) Possible null pointer dereference: p\n", errout.str());
+    }
+
     void functioncall() {    // #3443 - function calls
         // dereference pointer and then check if it's null
         {