Fixed cppcheck-opensource#4929 (False positive: possible null pointer deref (checks dont handle && and || well))

danmar · danmar · commit fd0f2d7900a6 · 2013-10-22T10:47:48.000+02:00
diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp
@@ -580,8 +580,8 @@ void CheckNullPointer::nullPointerStructByDeRefAndChec()
                     tok1 = tok1->next();
                 skipvar.insert(tok1->varId());
                 continue;
-            } else if (Token::Match(tok1, "( ! %var% %oror%") ||
-                       Token::Match(tok1, "( %var% &&")) {
+            } else if (Token::Match(tok1, "(|%oror% ! %var% %oror%") ||
+                       Token::Match(tok1, "(|&& %var% &&")) {
                 // TODO: there are false negatives caused by this. The
                 // variable should be removed from skipvar after the
                 // condition
@@ -631,6 +631,8 @@ void CheckNullPointer::nullPointerStructByDeRefAndChec()
                 const unsigned int varid1(tok1->varId());
                 if (varid1 == 0)
                     continue;
+                if (skipvar.find(varid1) != skipvar.end())
+                    continue;
                 const Token *tok2 = tok1->previous();
                 while (tok2 && !Token::Match(tok2, "[;{}]")) {
                     if (Token::Match(tok2, "%varid% =", varid1)) {
@@ -850,7 +852,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
                     // Don't write warning if the dereferencing is
                     // guarded by ?: or &&
                     const Token *tok2 = tok1->previous();
-                    if (tok2 && (tok2->isArithmeticalOp() || tok2->str() == "(")) {
+                    if (tok2 && (tok2->isArithmeticalOp() || Token::Match(tok2, "[(,]"))) {
                         while (tok2 && !Token::Match(tok2, "[;{}?:]")) {
                             if (tok2->str() == ")") {
                                 tok2 = tok2->link();
@@ -859,8 +861,8 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
                                     break;
                                 }
                             }
-                            // guarded by &&
-                            if (tok2->varId() == varid && tok2->next()->str() == "&&")
+                            // guarded by && or ||
+                            if (Token::Match(tok2, "%varid% &&|%oror%",varid))
                                 break;
                             tok2 = tok2->previous();
                         }
diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp
@@ -468,6 +468,24 @@ class TestNullPointer : public TestFixture {
               "}");
         ASSERT_EQUALS("", errout.str());
 
+        check("void f(ABC *abc) {\n"
+              "  x(def || !abc || y(def, abc->a));\n"
+              "  if (abc) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(ABC *abc) {\n"
+              "  x(abc && y(def, abc->a));\n"
+              "  if (abc) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(ABC *abc) {\n"
+              "    x(def && abc && y(def, abc->a));\n"
+              "    if (abc) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
         // #3228 - calling function with null object
         {
             const char code[] = "void f(Fred *fred) {\n"
