Fixed #4510 (False positive: "Possible null pointer dereference if the default parameter value is used" after init)

lib/checknullpointer.cpp

@@ -1194,6 +1194,26 @@ void CheckNullPointer::nullConstantDereference()
     }
 }
 
+/**
+* @brief If tok is a function call that passes in a pointer such that
+*         the pointer may be modified, this function will remove that
+*         pointer from pointerArgs.
+*/
+void CheckNullPointer::removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs)
+{
+    // Common functions that are known NOT to modify their pointer argument
+    const char safeFunctions[] = "printf|sprintf|fprintf|vprintf";
+
+    // If a pointer's address is passed into a function, stop considering it
+    if (Token::Match(tok->previous(), "[;{}] %var% (")) {
+        const Token* endParen = tok->next()->link();
+        for (const Token* tok2 = tok->next(); tok2 != endParen; tok2 = tok2->next()) {
+            if (tok2->isName() && tok2->varId() > 0 && !Token::Match(tok, safeFunctions)) {
+                pointerArgs.erase(tok2->varId());
+            }
+        }
+    }
+}
 
 /**
 * @brief Does one part of the check for nullPointer().
@@ -1223,8 +1243,8 @@ void CheckNullPointer::nullPointerDefaultArgument()
 
         // Report an error if any of the default-NULL arguments are dereferenced
         if (!pointerArgs.empty()) {
-            bool unknown = _settings->inconclusive;
             for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
+
                 // If we encounter a possible NULL-pointer check, skip over its body
                 if (Token::simpleMatch(tok, "if ( "))  {
                     bool dependsOnPointer = false;
@@ -1241,31 +1261,53 @@ void CheckNullPointer::nullPointerDefaultArgument()
                     // pointer check for the pointers referenced in its condition
                     const Token *endOfIf = startOfIfBlock->link();
                     bool isExitOrReturn =
-                        Token::findmatch(startOfIfBlock, "exit|return", endOfIf) != NULL;
+                        Token::findmatch(startOfIfBlock, "exit|return|throw", endOfIf) != NULL;
 
-                    for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) {
+                    if (Token::Match(tok, "if ( %var% == 0 )")) {
-                        if (tok2->isName() && tok2->varId() > 0 &&
+                        const unsigned int var = tok->tokAt(2)->varId();
-                            pointerArgs.count(tok2->varId()) > 0) {
+                        if (var > 0 && pointerArgs.count(var) > 0) {
-
-                            // If the if() depends on a pointer and may return, stop
-                            // considering that pointer because it may be a NULL-pointer
-                            // check that returns if the pointer is NULL.
                             if (isExitOrReturn)
-                                pointerArgs.erase(tok2->varId());
+                                pointerArgs.erase(var);
                             else
                                 dependsOnPointer = true;
                         }
+                    } else {
+                        for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) {
+                            if (tok2->isName() && tok2->varId() > 0 &&
+                                pointerArgs.count(tok2->varId()) > 0) {
+
+                                // If the if() depends on a pointer and may return, stop
+                                // considering that pointer because it may be a NULL-pointer
+                                // check that returns if the pointer is NULL.
+                                if (isExitOrReturn)
+                                    pointerArgs.erase(tok2->varId());
+                                else
+                                    dependsOnPointer = true;
+                            }
+                        }
                     }
+
                     if (dependsOnPointer && endOfIf) {
                         for (; tok != endOfIf; tok = tok->next()) {
                             // If a pointer is assigned a new value, stop considering it.
                             if (Token::Match(tok, "%var% ="))
                                 pointerArgs.erase(tok->varId());
+                            else
+                                removeAssignedVarFromSet(tok, pointerArgs);
                         }
                         continue;
                     }
                 }
 
+                // If there is a noreturn function (e.g. exit()), stop considering the rest of
+                // this function.
+                bool unknown = false;
+                if (Token::Match(tok, "return|throw|exit") ||
+                    (_tokenizer->IsScopeNoReturn(tok, &unknown) && !unknown))
+                    break;
+
+                removeAssignedVarFromSet(tok, pointerArgs);
+
                 if (tok->varId() == 0 || pointerArgs.count(tok->varId()) == 0)
                     continue;
 
@@ -1276,7 +1318,7 @@ void CheckNullPointer::nullPointerDefaultArgument()
                 // If a pointer dereference is preceded by an && or ||,
                 // they serve as a sequence point so the dereference
                 // may not be executed.
-                if (isPointerDeRef(tok, unknown) &&
+                if (isPointerDeRef(tok, unknown) && !unknown &&
                     tok->strAt(-1) != "&&" && tok->strAt(-1) != "||" &&
                     tok->strAt(-2) != "&&" && tok->strAt(-2) != "||")
                     nullPointerDefaultArgError(tok, tok->str());

lib/checknullpointer.h

@@ -152,6 +152,11 @@ private:
      */
     void nullPointerDefaultArgument();
 
+    /**
+     * @brief Removes any variable that may be assigned from pointerArgs.
+     */
+    void removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs);
+
     /**
      * @brief Investigate if function call can make pointer null. If
      * the pointer is passed by value it can't be made a null pointer.

test/testnullpointer.cpp

@@ -2110,6 +2110,13 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
 
+        check("void f(int *p = 0) {\n"
+              "    if (!p)\n"
+              "        return;\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
         check("void f(char a, int *p = 0) {\n"
               "    *p = 0;\n"
               "}");
@@ -2195,6 +2202,53 @@ private:
               "    return !p || *p;\n"
               "}");
         ASSERT_EQUALS("", errout.str());
+
+
+        // bar may initialize p but be can't know for sure without knowing
+        // if p is passed in by reference and is modified by bar()
+        check("void f(int *p = 0) {\n"
+              "    bar(p);\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(int *p = 0) {\n"
+              "    printf(\"%d\", p);\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
+
+        // The init() function may or may not initialize p, but since the address
+        // of p is passed in, it's a good bet that p may be modified and
+        // so we should not report an error.
+        check("void f(int *p = 0) {\n"
+              "    init(&p);\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void init(int* &g);\n"
+              "void f(int *p = 0) {\n"
+              "    init(p);\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(int *p = 0) {\n"
+              "    if (p == 0) {\n"
+              "        init(&p);\n"
+              "    }\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(int *p = 0) {\n"
+              "    if (p == 0) {\n"
+              "        throw SomeException;\n"
+              "    }\n"
+              "    *p = 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
     }