diff --git a/lib/checkother.cpp b/lib/checkother.cpp index be64de639..1ae42fe2f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2253,6 +2253,7 @@ void CheckOther::checkDoubleFree() std::set closeDirVariables; for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) { + bool isUnknown = true; // Keep track of any variables passed to "free()", "g_free()" or "closedir()", // and report an error if the same variable is passed twice. @@ -2287,15 +2288,19 @@ void CheckOther::checkDoubleFree() } // If this scope doesn't return, clear the set of previously freed variables - else if (tok->str() == "}" && _tokenizer->IsScopeNoReturn(tok)) { + else if (tok->str() == "}" && _tokenizer->IsScopeNoReturn(tok, &isUnknown) && !isUnknown) { freedVariables.clear(); closeDirVariables.clear(); } - // If this scope is a "for" or "while" loop, give up on trying to figure - // out the flow of execution and just clear the set of previously freed variables - else if (tok->str() == "}" && tok->link() && tok->link()->previous() && tok->link()->previous()->link() && - Token::Match(tok->link()->previous()->link()->previous(), "while|for")) { + // If this scope is a "for" or "while" loop that contains "break" or "continue", + // give up on trying to figure out the flow of execution and just clear the set + // of previously freed variables. + // TODO: There are false negatives. This bailout is only needed when the + // loop will exit without free()'ing the memory on the last iteration. + else if (tok->str() == "}" && tok->link() && tok->link()->linkAt(-1) && + Token::Match(tok->link()->linkAt(-1)->previous(), "while|for") && + Token::findmatch(tok->link()->linkAt(-1), "break|continue ;", tok) != NULL) { freedVariables.clear(); closeDirVariables.clear(); } diff --git a/test/testother.cpp b/test/testother.cpp index e0462beb8..8894a7382 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -4925,7 +4925,7 @@ private: " char *p = malloc(100);\n" " if (x) {\n" " free(p);\n" - " bailout();\n" + " exit();\n" " }\n" " free(p);\n" "}"); @@ -5103,6 +5103,45 @@ private: ); ASSERT_EQUALS("", errout.str()); + check( + "void foo(int y)\n" + "{\n" + " char * x = NULL;\n" + " for (int i = 0; i < 10000; i++) {\n" + " x = new char[100];\n" + " delete[] x;\n" + " }\n" + " delete[] x;\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); + + check( + "void foo(int y)\n" + "{\n" + " char * x = NULL;\n" + " while (isRunning()) {\n" + " x = new char[100];\n" + " delete[] x;\n" + " }\n" + " delete[] x;\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); + + check( + "void foo(int y)\n" + "{\n" + " char * x = NULL;\n" + " while (isRunning()) {\n" + " x = malloc(100);\n" + " free(x);\n" + " }\n" + " free(x);\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); + check( "void foo(int y)\n" "{\n" @@ -5132,6 +5171,19 @@ private: "}" ); ASSERT_EQUALS("", errout.str()); + + check( + "void f()\n" + "{\n" + " char *p = 0;\n" + " if (x < 100) {\n" + " p = malloc(10);\n" + " free(p);\n" + " }\n" + " free(p);\n" + "}" + ); + ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); } };