Fixed Ticket #3300 (false negative: doublefree of pointer)

lib/checkother.cpp

@@ -2486,6 +2486,96 @@ void CheckOther::checkDuplicateBranch()
     }
 }
 
+//-----------------------------------------------------------------------------
+// Check for double free
+// free(p); free(p);
+//-----------------------------------------------------------------------------
+void CheckOther::checkDoubleFree()
+{
+    std::set<int> freedVariables;
+    std::set<int> closeDirVariables;
+
+    for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) {
+
+        // Keep track of any variables passed to "free()", "g_free()" or "closedir()",
+        // and report an error if the same variable is passed twice.
+        if (Token::Match(tok, "free|g_free|closedir ( %var% )")) {
+            int var = tok->tokAt(2)->varId();
+            if (var) {
+                if (Token::Match(tok, "free|g_free")) {
+                    if (freedVariables.find(var) != freedVariables.end())
+                        doubleFreeError(tok, tok->tokAt(2)->str());
+                    else
+                        freedVariables.insert(var);
+                } else if (tok->str() == "closedir") {
+                    if (closeDirVariables.find(var) != closeDirVariables.end())
+                        doubleCloseDirError(tok, tok->tokAt(2)->str());
+                    else
+                        closeDirVariables.insert(var);
+                }
+            }
+        }
+
+        // Keep track of any variables operated on by "delete" or "delete[]"
+        // and report an error if the same variable is delete'd twice.
+        else if (Token::Match(tok, "delete %var% ;") || Token::Match(tok, "delete [ ] %var% ;")) {
+            int varIdx = (tok->strAt(1) == "[") ? 3 : 1;
+            int var = tok->tokAt(varIdx)->varId();
+            if (var) {
+                if (freedVariables.find(var) != freedVariables.end())
+                    doubleFreeError(tok, tok->tokAt(varIdx)->str());
+                else
+                    freedVariables.insert(var);
+            }
+        }
+
+        // If this scope doesn't return, clear the set of previously freed variables
+        else if (tok->str() == "}" && _tokenizer->IsScopeNoReturn(tok)) {
+            freedVariables.clear();
+            closeDirVariables.clear();
+        }
+
+        // If a variable is passed to a function, remove it from the set of previously freed variables
+        else if (Token::Match(tok, "%var% (") && !Token::Match(tok, "printf|sprintf|snprintf|fprintf")) {
+            for (const Token* tok2 = tok->tokAt(2); tok2 != tok->linkAt(1); tok2 = tok2->next()) {
+                if (Token::Match(tok2, "%var%")) {
+                    int var = tok2->varId();
+                    if (var) {
+                        freedVariables.erase(var);
+                        closeDirVariables.erase(var);
+                    }
+                }
+            }
+        }
+
+        // If a pointer is assigned a new value, remove it from the set of previously freed variables
+        else if (Token::Match(tok, "%var% =")) {
+            int var = tok->varId();
+            if (var) {
+                freedVariables.erase(var);
+                closeDirVariables.erase(var);
+            }
+        }
+
+        // Any control statements in-between delete, free() or closedir() statements
+        // makes it unclear whether any subsequent statements would be redundant.
+        if (Token::Match(tok, "else|break|continue|goto|return")) {
+            freedVariables.clear();
+            closeDirVariables.clear();
+        }
+    }
+}
+
+void CheckOther::doubleFreeError(const Token *tok, const std::string &varname)
+{
+    reportError(tok, Severity::error, "doubleFree", "Memory pointed to by '" + varname +"' is freed twice.");
+}
+
+void CheckOther::doubleCloseDirError(const Token *tok, const std::string &varname)
+{
+    reportError(tok, Severity::error, "doubleCloseDir", "Directory handle '" + varname +"' closed twice.");
+}
+
 void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2)
 {
     std::list<const Token *> toks;

lib/checkother.h

@@ -107,6 +107,7 @@ public:
         checkOther.checkAssignBoolToPointer();
         checkOther.checkSignOfUnsignedVariable();
         checkOther.checkBitwiseOnBoolean();
+        checkOther.checkDoubleFree();
     }
 
     /** @brief Clarify calculation for ".. a * b ? .." */
@@ -261,6 +262,9 @@ public:
     /** @brief %Check for suspicious use of semicolon */
     void checkSuspiciousSemicolon();
 
+    /** @brief %Check for double free or double close operations */
+    void checkDoubleFree();
+
     // Error messages..
     void cstyleCastError(const Token *tok);
     void dangerousUsageStrtolError(const Token *tok);
@@ -307,6 +311,8 @@ public:
     void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op);
     void comparisonOfBoolExpressionWithIntError(const Token *tok);
     void SuspiciousSemicolonError(const Token *tok);
+    void doubleFreeError(const Token *tok, const std::string &varname);
+    void doubleCloseDirError(const Token *tok, const std::string &varname);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) {
         CheckOther c(0, settings, errorLogger);
@@ -391,6 +397,7 @@ public:
                "* incorrect length arguments for 'substr' and 'strncmp'\n"
                "* invalid usage of output stream. For example: std::cout << std::cout;'\n"
                "* wrong number of arguments given to 'printf' or 'scanf;'\n"
+               "* double free() or double closedir()\n"
 
                // style
                "* C-style pointer cast in cpp file\n"

test/testother.cpp

@@ -153,6 +153,8 @@ private:
 
         TEST_CASE(checkForSuspiciousSemicolon1);
         TEST_CASE(checkForSuspiciousSemicolon2);
+
+        TEST_CASE(checkDoubleFree);
     }
 
     void check(const char code[], const char *filename = NULL, bool experimental = false) {
@@ -175,7 +177,6 @@ private:
 
         // Simplify token list..
         tokenizer.simplifyTokenList();
-
         checkOther.runSimplifiedChecks(&tokenizer, &settings, this);
     }
 
@@ -4451,6 +4452,225 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void checkDoubleFree() {
+        check(
+            "void foo(char *p) {\n"
+            "  free(p);\n"
+            "  free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p, char *r) {\n"
+            "  free(p);\n"
+            "  free(r);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo() {\n"
+            "  free(p);\n"
+            "  free(r);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  if (x < 3) free(p);\n"
+            "  else if (x > 9) free(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  free(p);\n"
+            "  getNext(&p);\n"
+            "  free(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  free(p);\n"
+            "  bar();\n"
+            "  free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  free(p);\n"
+            "  printf(\"Freed memory at location %x\", (unsigned int) p);\n"
+            "  free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(DIR *p) {\n"
+            "  closedir(p);\n"
+            "  closedir(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Directory handle 'p' closed twice.\n", errout.str());
+
+        check(
+            "void foo(DIR *p, DIR *r) {\n"
+            "  closedir(p);\n"
+            "  closedir(r);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(DIR *p) {\n"
+            "  if (x < 3) closedir(p);\n"
+            "  else if (x > 9) closedir(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(DIR *p) {\n"
+            "  closedir(p);\n"
+            "  gethandle(&p);\n"
+            "  closedir(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(DIR *p) {\n"
+            "  closedir(p);\n"
+            "  gethandle();\n"
+            "  closedir(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Directory handle 'p' closed twice.\n", errout.str());
+
+        check(
+            "void foo(Data* p) {\n"
+            "  free(p->a);\n"
+            "  free(p->b);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void f() {\n"
+            "    char *p = malloc(100);\n"
+            "    if (x) {\n"
+            "        free(p);\n"
+            "        bailout();\n"
+            "    }\n"
+            "    free(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void f() {\n"
+            "    char *p = malloc(100);\n"
+            "    if (x) {\n"
+            "        free(p);\n"
+            "		 x = 0;\n"
+            "    }\n"
+            "    free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:7]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void f() {\n"
+            "    char *p = do_something();\n"
+            "    free(p);\n"
+            "    p = do_something();\n"
+            "    free(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  g_free(p);\n"
+            "  g_free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p, char *r) {\n"
+            "  g_free(p);\n"
+            "  g_free(r);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  g_free(p);\n"
+            "  getNext(&p);\n"
+            "  g_free(p);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  g_free(p);\n"
+            "  bar();\n"
+            "  g_free(p);\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete p;\n"
+            "  delete p;\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p, char *r) {\n"
+            "  delete p;\n"
+            "  delete r;\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete p;\n"
+            "  getNext(&p);\n"
+            "  delete p;\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete p;\n"
+            "  bar();\n"
+            "  delete p;\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete[] p;\n"
+            "  delete[] p;\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+
+        check(
+            "void foo(char *p, char *r) {\n"
+            "  delete[] p;\n"
+            "  delete[] r;\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete[] p;\n"
+            "  getNext(&p);\n"
+            "  delete[] p;\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "void foo(char *p) {\n"
+            "  delete[] p;\n"
+            "  bar();\n"
+            "  delete[] p;\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
+    }
+
     void coutCerrMisusage() {
         check(
             "void foo() {\n"