From b89adff9fd264e23dc30f59d7b53a3b876165e2d Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Sat, 14 Jan 2012 16:19:34 -0800 Subject: [PATCH] Fixed Ticket #3300 (false negative: doublefree of pointer) --- lib/checkother.cpp | 90 ++++++++++++++++++ lib/checkother.h | 7 ++ test/testother.cpp | 222 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 318 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 3745a2f6b..4a8830889 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2486,6 +2486,96 @@ void CheckOther::checkDuplicateBranch() } } +//----------------------------------------------------------------------------- +// Check for double free +// free(p); free(p); +//----------------------------------------------------------------------------- +void CheckOther::checkDoubleFree() +{ + std::set freedVariables; + std::set 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 toks; diff --git a/lib/checkother.h b/lib/checkother.h index 52ab7a913..bd4a9698d 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index ef5a6701c..82e7d2159 100644 --- a/test/testother.cpp +++ b/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"