From abceff497bb448996b5b92c31654886d1a01d0a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 4 Apr 2010 11:24:52 +0200 Subject: [PATCH] Refactoring: some refactoring of ExecutionPath. The foundError was removed. No automatic bailout of all checks are made when errors are found. --- lib/checkmemoryleak.cpp | 10 ++-- lib/checkother.cpp | 127 ++++++++++++++++++++-------------------- lib/executionpath.cpp | 57 ++++++------------ lib/executionpath.h | 2 +- test/testother.cpp | 4 +- 5 files changed, 90 insertions(+), 110 deletions(-) diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index 8f91db548..891f08a34 100644 --- a/lib/checkmemoryleak.cpp +++ b/lib/checkmemoryleak.cpp @@ -2747,7 +2747,7 @@ private: } /** return */ - static void ret(const std::list &checks, const Token *tok, bool &foundError) + static void ret(const std::list &checks, const Token *tok) { std::list::const_iterator it; for (it = checks.begin(); it != checks.end(); ++it) @@ -2759,7 +2759,6 @@ private: if (checkMemleak) { checkMemleak->memleakError(tok, C->varname, false); - foundError = true; break; } } @@ -2767,7 +2766,7 @@ private: } /** parse the tokens */ - const Token *parse(const Token &tok, bool &foundError, std::list &checks) const + const Token *parse(const Token &tok, std::list &checks) const { //std::cout << "CheckLocalLeaks::parse " << tok.str() << std::endl; //printOut(checks); @@ -2808,7 +2807,7 @@ private: if (tok.str() == "return") { - ret(checks, &tok, foundError); + ret(checks, &tok); } return &tok; @@ -2817,8 +2816,7 @@ private: /** going out of scope - all execution paths end */ void end(const std::list &checks, const Token *tok) const { - bool foundError = false; - ret(checks, tok, foundError); + ret(checks, tok); } }; diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 27c8cc325..e5535de27 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1474,8 +1474,12 @@ private: } } - /** variable is dereferenced. If the variable is null there's an error */ - static void dereference(bool &foundError, std::list &checks, const Token *tok) + /** + * Dereferencing variable. Check if it is safe (if the variable is null there's an error) + * @param checks Checks + * @param tok token where dereferencing happens + */ + static void dereference(std::list &checks, const Token *tok) { const unsigned int varid(tok->varId()); @@ -1485,19 +1489,18 @@ private: CheckNullpointer *c = dynamic_cast(*it); if (c && c->varId == varid && c->null) { - foundError = true; CheckOther *checkOther = dynamic_cast(c->owner); if (checkOther) { checkOther->nullPointerError(tok, c->varname); - break; + return; } } } } /** parse tokens */ - const Token *parse(const Token &tok, bool &foundError, std::list &checks) const + const Token *parse(const Token &tok, std::list &checks) const { if (Token::Match(tok.previous(), "[;{}] const| %type% * %var% ;")) { @@ -1543,7 +1546,7 @@ private: std::list var; parseFunctionCall(tok, var, 0); for (std::list::const_iterator it = var.begin(); it != var.end(); ++it) - dereference(foundError, checks, *it); + dereference(checks, *it); } if (tok.varId() != 0) @@ -1551,17 +1554,17 @@ private: if (Token::Match(tok.previous(), "[;{}=] %var% = 0 ;")) setnull(checks, tok.varId()); else if (Token::Match(tok.tokAt(-2), "[;{}=+-/(,] * %var%")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else if (Token::Match(tok.tokAt(-2), "return * %var%")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else if (!Token::simpleMatch(tok.tokAt(-2), "& (") && Token::Match(tok.next(), ". %var%")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else if (Token::Match(tok.previous(), "[;{}=+-/(,] %var% [ %any% ]")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else if (Token::Match(tok.previous(), "return %var% [ %any% ]")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else if (Token::Match(&tok, "%var% (")) - dereference(foundError, checks, &tok); + dereference(checks, &tok); else bailOutVar(checks, tok.varId()); } @@ -1575,7 +1578,6 @@ private: if (checkOther) { checkOther->nullPointerError(&tok); - foundError = true; } } } @@ -1588,11 +1590,10 @@ private: { if (Token::Match(&tok, "!| %var% (")) { - bool foundError = false; std::list var; parseFunctionCall(tok.str() == "!" ? *tok.next() : tok, var, 0); for (std::list::const_iterator it = var.begin(); it != var.end(); ++it) - dereference(foundError, checks, *it); + dereference(checks, *it); } return ExecutionPath::parseCondition(tok, checks); @@ -1656,7 +1657,7 @@ private: } /** Initializing a pointer value. For example: *p = 0; */ - static void init_pointer(bool &foundError, std::list &checks, const Token *tok) + static void init_pointer(std::list &checks, const Token *tok) { const unsigned int varid(tok->varId()); if (!varid) @@ -1676,7 +1677,7 @@ private: } else { - use_pointer(foundError, checks, tok); + use_pointer(checks, tok); } } @@ -1685,7 +1686,7 @@ private: } /** Deallocate a pointer. For example: free(p); */ - static void dealloc_pointer(bool &foundError, std::list &checks, const Token *tok) + static void dealloc_pointer(std::list &checks, const Token *tok) { const unsigned int varid(tok->varId()); if (!varid) @@ -1702,7 +1703,6 @@ private: CheckOther *checkOther = dynamic_cast(c->owner); if (checkOther) { - foundError = true; checkOther->uninitvarError(tok, c->varname); break; } @@ -1777,16 +1777,16 @@ private: /** * use - called from the use* functions below. - * @param foundError this is set to true if an error is found * @param checks all available checks * @param tok variable token * @param mode specific behaviour + * @return if error is found, true is returned */ - static void use(bool &foundError, std::list &checks, const Token *tok, const int mode) + static bool use(std::list &checks, const Token *tok, const int mode) { const unsigned int varid(tok->varId()); if (varid == 0) - return; + return false; std::list::const_iterator it; for (it = checks.begin(); it != checks.end(); ++it) @@ -1819,56 +1819,55 @@ private: checkOther->uninitdataError(tok, c->varname); else checkOther->uninitvarError(tok, c->varname); - foundError = true; - break; + return true; } } } + + // No error found + return false; } /** * Reading variable. Use this function in situations when it is * invalid to read the data of the variable but not the address. - * @param foundError this is set to true if an error is found * @param checks all available checks * @param tok variable token + * @return if error is found, true is returned */ - static void use(bool &foundError, std::list &checks, const Token *tok) + static bool use(std::list &checks, const Token *tok) { - use(foundError, checks, tok, 0); + return use(checks, tok, 0); } /** * Reading array elements. If the variable is not an array then the usage is ok. - * @param foundError this is set to true if an error is found * @param checks all available checks * @param tok variable token */ - static void use_array(bool &foundError, std::list &checks, const Token *tok) + static void use_array(std::list &checks, const Token *tok) { - use(foundError, checks, tok, 1); + use(checks, tok, 1); } /** * Bad pointer usage. If the variable is not a pointer then the usage is ok. - * @param foundError this is set to true if an error is found * @param checks all available checks * @param tok variable token */ - static void use_pointer(bool &foundError, std::list &checks, const Token *tok) + static void use_pointer(std::list &checks, const Token *tok) { - use(foundError, checks, tok, 2); + use(checks, tok, 2); } /** * Using variable.. if it's a dead pointer the usage is invalid. - * @param foundError this is set to true if an error is found * @param checks all available checks * @param tok variable token */ - static void use_dead_pointer(bool &foundError, std::list &checks, const Token *tok) + static void use_dead_pointer(std::list &checks, const Token *tok) { - use(foundError, checks, tok, 3); + use(checks, tok, 3); } /** declaring a variable */ @@ -1916,7 +1915,7 @@ private: } /** parse tokens. @sa ExecutionPath::parse */ - const Token *parse(const Token &tok, bool &foundError, std::list &checks) const + const Token *parse(const Token &tok, std::list &checks) const { // Variable declaration.. if (tok.str() != "return") @@ -1967,13 +1966,13 @@ private: // Used.. if (Token::Match(tok.previous(), "[[(,+-*/] %var% []),+-*/]")) { - use(foundError, checks, &tok); + use(checks, &tok); return &tok; } if (Token::Match(tok.previous(), "++|--") || Token::Match(tok.next(), "++|--")) { - use(foundError, checks, &tok); + use(checks, &tok); return &tok; } @@ -1989,7 +1988,13 @@ private: if (tok2->varId() && !Token::Match(tok2->previous(), "&|::") && !Token::simpleMatch(tok2->next(), "=")) - use(foundError, checks, tok2); + { + bool foundError = use(checks, tok2); + if (foundError) + { + bailOutVar(checks, tok2->varId()); + } + } } // pointer aliasing? @@ -2001,15 +2006,15 @@ private: if (Token::simpleMatch(tok.next(), "(")) { - use_pointer(foundError, checks, &tok); + use_pointer(checks, &tok); } if (Token::Match(tok.tokAt(-2), "[;{}] *")) { if (Token::simpleMatch(tok.next(), "=")) - init_pointer(foundError, checks, &tok); + init_pointer(checks, &tok); else - use_pointer(foundError, checks, &tok); + use_pointer(checks, &tok); return &tok; } @@ -2025,11 +2030,11 @@ private: if (Token::Match(eq, "+=|-=|*=|/=|&=|^=") || eq->str() == "|=") { if (tok.previous()->str() == "*") - use_pointer(foundError, checks, &tok); + use_pointer(checks, &tok); else if (tok.next()->str() == "[") - use_array(foundError, checks, &tok); + use_array(checks, &tok); else - use(foundError, checks, &tok); + use(checks, &tok); } } @@ -2059,7 +2064,7 @@ private: if (Token::simpleMatch(tok.previous(), "delete") || Token::simpleMatch(tok.tokAt(-3), "delete [ ]")) { - dealloc_pointer(foundError, checks, &tok); + dealloc_pointer(checks, &tok); return &tok; } } @@ -2072,7 +2077,7 @@ private: // deallocate pointer if (Token::Match(&tok, "free|kfree|fclose ( %var% )")) { - dealloc_pointer(foundError, checks, tok.tokAt(2)); + dealloc_pointer(checks, tok.tokAt(2)); return tok.tokAt(3); } @@ -2081,7 +2086,7 @@ private: std::list var; parseFunctionCall(tok, var, 1); for (std::list::const_iterator it = var.begin(); it != var.end(); ++it) - use_array(foundError, checks, *it); + use_array(checks, *it); } // strncpy doesn't 0-terminate first parameter @@ -2122,7 +2127,7 @@ private: else if (tok2->varId()) { if (Token::Match(tok2->tokAt(-2), "[(,] *") || Token::Match(tok2->next(), ". %var%")) - use_dead_pointer(foundError, checks, tok2); + use_dead_pointer(checks, tok2); // it is possible that the variable is initialized here bailouts.insert(tok2->varId()); @@ -2163,7 +2168,7 @@ private: // Todo: if (!array && .. if (Token::Match(tok.next(), "%var% ;")) { - use(foundError, checks, tok.next()); + use(checks, tok.next()); } } @@ -2181,7 +2186,7 @@ private: { if (!Token::Match(tok.tokAt(-3), "[;{}] %var% =")) { - use(foundError, checks, &tok); + use(checks, &tok); return &tok; } @@ -2193,7 +2198,7 @@ private: if (tok2 && !Token::simpleMatch(tok2->previous(), "*")) */ { - use(foundError, checks, &tok); + use(checks, &tok); return &tok; } } @@ -2206,7 +2211,7 @@ private: while (Token::Match(tok2, ". %var%")) tok2 = tok2->tokAt(2); if (tok2 && tok2->str() != "=") - use_pointer(foundError, checks, &tok); + use_pointer(checks, &tok); else bailOutVar(checks, tok.varId()); return &tok; @@ -2220,7 +2225,7 @@ private: if (Token::Match(tok.tokAt(-2), "[,(=] *")) { - use_pointer(foundError, checks, &tok); + use_pointer(checks, &tok); return &tok; } @@ -2254,7 +2259,7 @@ private: { // If the variable hasn't been initialized then call "use" if (varid1.find(tok2->next()->varId()) == varid1.end()) - use(foundError, checks, tok2->next()); + use(checks, tok2->next()); } // goto stepcode @@ -2302,7 +2307,7 @@ private: tok2 = tok2->next(); // call "use" - use(foundError, checks, tok2); + use(checks, tok2); } } } @@ -2313,22 +2318,20 @@ private: bool parseCondition(const Token &tok, std::list &checks) { - bool foundError = false; - if (tok.varId() && Token::Match(&tok, "%var% <|<=|==|!=|)|[")) - use(foundError, checks, &tok); + use(checks, &tok); else if (Token::Match(&tok, "!| %var% (")) { std::list var; parseFunctionCall(tok.str() == "!" ? *tok.next() : tok, var, 1); for (std::list::const_iterator it = var.begin(); it != var.end(); ++it) - use_array(foundError, checks, *it); + use_array(checks, *it); } else if (Token::Match(&tok, "! %var% )")) { - use(foundError, checks, &tok); + use(checks, &tok); return false; } diff --git a/lib/executionpath.cpp b/lib/executionpath.cpp index 4b7565ec2..86d09941c 100644 --- a/lib/executionpath.cpp +++ b/lib/executionpath.cpp @@ -56,17 +56,17 @@ bool ExecutionPath::parseCondition(const Token &tok, std::list } -static const Token *checkExecutionPaths_(const Token *tok, std::list &checks) +static void checkExecutionPaths_(const Token *tok, std::list &checks) { if (!tok || tok->str() == "}" || checks.empty()) - return 0; + return; const std::auto_ptr check(checks.front()->copy()); for (; tok; tok = tok->next()) { if (tok->str() == "}") - return 0; + return; if (Token::simpleMatch(tok, "while (")) { @@ -74,7 +74,7 @@ static const Token *checkExecutionPaths_(const Token *tok, std::list 10 || check->parseCondition(*tok->tokAt(2), checks)) { ExecutionPath::bailOut(checks); - return 0; + return; } // skip "while (fgets()!=NULL)" @@ -95,7 +95,7 @@ static const Token *checkExecutionPaths_(const Token *tok, std::liststr() == "goto") { ExecutionPath::bailOut(checks); - return 0; + return; } // for/while/switch/do .. bail out @@ -110,7 +110,7 @@ static const Token *checkExecutionPaths_(const Token *tok, std::liststr() != "{") { ExecutionPath::bailOut(checks); - return 0; + return; } // skip { .. } @@ -137,13 +137,13 @@ static const Token *checkExecutionPaths_(const Token *tok, std::listtokAt(2), ". %var% =")) { ExecutionPath::bailOut(checks); - return 0; + return; } tok = tok->next()->link(); if (!tok) { ExecutionPath::bailOut(checks); - return 0; + return; } continue; } @@ -175,12 +175,7 @@ static const Token *checkExecutionPaths_(const Token *tok, std::listprevious(), "[;{}] {")) { - const Token *tokerr = checkExecutionPaths_(tok->next(), checks); - if (tokerr) - { - ExecutionPath::bailOut(checks); - return tokerr; - } + checkExecutionPaths_(tok->next(), checks); tok = tok->link(); continue; } @@ -198,7 +193,7 @@ static const Token *checkExecutionPaths_(const Token *tok, std::list::iterator it; for (it = checks.begin(); it != checks.end(); ++it) c.push_back((*it)->copy()); - const Token *tokerr = checkExecutionPaths_(tok->next(), c); - if (tokerr) - { - ExecutionPath::bailOut(c); - ExecutionPath::bailOut(newchecks); - return tokerr; - } + checkExecutionPaths_(tok->next(), c); while (!c.empty()) { newchecks.push_back(c.back()); @@ -247,18 +236,12 @@ static const Token *checkExecutionPaths_(const Token *tok, std::listnext(), checks); - if (tokerr) - { - ExecutionPath::bailOut(newchecks); - return tokerr; - } - + checkExecutionPaths_(tok->next(), checks); tok = tok->link(); if (!tok) { ExecutionPath::bailOut(newchecks); - return 0; + return; } } @@ -269,12 +252,9 @@ static const Token *checkExecutionPaths_(const Token *tok, std::listparse(*tok, foundError, checks); + tok = check->parse(*tok, checks); if (checks.empty()) - return 0; - else if (foundError) - return tok; + return; } // return/throw ends all execution paths @@ -283,7 +263,6 @@ static const Token *checkExecutionPaths_(const Token *tok, std::list &checks) const = 0; + virtual const Token *parse(const Token &tok, std::list &checks) const = 0; /** * Parse condition diff --git a/test/testother.cpp b/test/testother.cpp index cf01642a5..37d89b038 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1040,8 +1040,8 @@ private: " int r = *p;\n" " int r2 = *p2;\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Null pointer dereference\n" - "[test.cpp:6]: (error) Null pointer dereference\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Null pointer dereference\n" + "[test.cpp:6]: (error) Null pointer dereference\n", errout.str()); }