diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 3ce606108..07e5c1468 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2409,6 +2409,63 @@ void CheckOther::checkDuplicateBranch() } } + +//----------------------------------------------------------------------------- +// Check for a free() of an invalid address +// char* p = malloc(100); +// free(p + 10); +//----------------------------------------------------------------------------- +void CheckOther::checkInvalidFree() +{ + std::set allocatedVariables; + for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) { + + // Keep track of which variables were assigned addresses to newly-allocated memory + if (Token::Match(tok, "%var% = malloc|g_malloc|new")) { + allocatedVariables.insert(tok->varId()); + } + + // If these variables assigned new values before being used to free the memory, we can't + // say anything about whether the resulting expression is valid + else if (Token::Match(tok, "%var% =")) { + allocatedVariables.erase(tok->varId()); + } + + // If a variable that was previously assigned a newly-allocated memory location is + // added or subtracted from when used to free the memory, report an error. + else if (Token::Match(tok, "free|g_free|delete ( %any% +|- %any%") || + Token::Match(tok, "delete [ ] ( %any% +|- %any%") || + Token::Match(tok, "delete %any% +|- %any%")) { + + int varIdx = tok->strAt(1) == "(" ? 2 : + tok->strAt(3) == "(" ? 4 : 1; + unsigned int var1 = tok->tokAt(varIdx)->varId(); + unsigned int var2 = tok->tokAt(varIdx + 2)->varId(); + if (allocatedVariables.find(var1) != allocatedVariables.end() || + allocatedVariables.find(var2) != allocatedVariables.end()) { + invalidFreeError(tok); + } + } + + // If the previously-allocated variable is passed in to another function + // as a parameter, it might be modified, so we shouldn't report an error + // if it is later used to free memory + else if (Token::Match(tok, "%var% (")) { + const Token* tok2 = Token::findmatch(tok->tokAt(1), "%var%", tok->linkAt(1)); + while (tok2 != NULL) { + allocatedVariables.erase(tok2->varId()); + tok2 = Token::findmatch(tok2->next(), "%var%", tok->linkAt(1)); + } + } + } +} + +void CheckOther::invalidFreeError(const Token *tok) +{ + reportError(tok, Severity::error, "invalidFree", "Invalid memory address freed."); +} + + //----------------------------------------------------------------------------- // Check for double free // free(p); free(p); diff --git a/lib/checkother.h b/lib/checkother.h index 3ab32d658..b926ae2af 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -106,6 +106,7 @@ public: checkOther.checkAssignBoolToPointer(); checkOther.checkBitwiseOnBoolean(); + checkOther.checkInvalidFree(); checkOther.checkDoubleFree(); checkOther.checkRedundantCopy(); checkOther.checkNegativeBitwiseShift(); @@ -247,6 +248,10 @@ public: /** @brief %Check for suspicious use of semicolon */ void checkSuspiciousSemicolon(); + /** @brief %Check for free() operations on invalid memory locations */ + void checkInvalidFree(); + void invalidFreeError(const Token *tok); + /** @brief %Check for double free or double close operations */ void checkDoubleFree(); void doubleFreeError(const Token *tok, const std::string &varname); @@ -407,6 +412,7 @@ private: "* sizeof for numeric given as function argument\n" "* using sizeof(pointer) instead of the size of pointed data\n" "* incorrect length arguments for 'substr' and 'strncmp'\n" + "* free() or delete of an invalid memory location\n" "* double free() or double closedir()\n" "* bitwise operation with negative right operand\n" diff --git a/test/testother.cpp b/test/testother.cpp index 44b0bad98..071b4230b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -162,6 +162,7 @@ private: TEST_CASE(checkForSuspiciousSemicolon2); TEST_CASE(checkDoubleFree); + TEST_CASE(checkInvalidFree); TEST_CASE(checkRedundantCopy); @@ -5864,6 +5865,89 @@ private: ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); } + + void checkInvalidFree() { + check( + "void foo(char *p) {\n" + " char *a = (char *)malloc(1024);\n" + " free(a + 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = (char *)malloc(1024);\n" + " free(a - 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = (char *)malloc(1024);\n" + " free(10 + a);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char[1024];\n" + " delete[] (a + 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " delete (a + 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); + + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " a = a - 10;\n" + " delete (a + 10);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " bar(a);\n" + " delete (a + 10);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " char *b = new char;\n" + " bar(a);\n" + " delete (a + 10);\n" + " delete (b + 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:6]: (error) Invalid memory address freed.\n", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " char *b = new char;\n" + " bar(a, b);\n" + " delete (a + 10);\n" + " delete (b + 10);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check( + "void foo(char *p) {\n" + " char *a = new char;\n" + " bar()\n" + " delete (a + 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Invalid memory address freed.\n", errout.str()); + } + void check_redundant_copy(const char code[]) { // Clear the error buffer.. errout.str("");