From 42a75692d41e1dc0e87e860b1e8290d1fb167d59 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 25 Jan 2012 15:16:22 +0100 Subject: [PATCH] Improved nullpointer check: - More accurate checking for dereferences and non-dereferences - improved checking for nullpointer dereferences after return statement - Supports pointer dereferences by std::string - Code optimization/refactorization --- lib/checknullpointer.cpp | 205 +++++++++++++++++++++------------------ lib/checknullpointer.h | 3 +- lib/checkuninitvar.cpp | 2 +- test/testnullpointer.cpp | 52 ++++++++++ 4 files changed, 163 insertions(+), 99 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 432f2b319..274a3b2e9 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -129,7 +129,7 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::listnextArgument(); if (secondParameter && ((value == 0 && secondParameter->str() == "0") || (Token::Match(secondParameter, "%var%") && secondParameter->varId() > 0))) { if (functionNames2_all.find(tok.str()) != functionNames2_all.end()) @@ -216,14 +216,14 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::list there is a dereference */ -bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) +bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const SymbolDatabase* symbolDatabase) { const bool inconclusive = unknown; unknown = false; // Dereferencing pointer.. - if (Token::Match(tok->tokAt(-3), "!!sizeof [;{}=+-/(,] * %var%") && Token::Match(tok->tokAt(-3), "!!decltype [;{}=+-/(,] * %var%")) + if (tok->strAt(-1) == "*" && (Token::Match(tok->tokAt(-2), "return|throw|;|{|}|:|[|(|,") || tok->tokAt(-2)->isOp() || tok->tokAt(-2)->isAssignmentOp()) && !Token::Match(tok->tokAt(-3), "sizeof|decltype")) return true; // read/write member variable @@ -234,7 +234,7 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) return false; } - if (Token::Match(tok->previous(), "!!& %var% [")) + if (Token::Match(tok, "%var% [") && (tok->previous()->str() != "&" || Token::Match(tok->next()->link()->next(), "[.(]"))) return true; if (Token::Match(tok, "%var% (")) @@ -245,6 +245,23 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) tok->varId() == tok->tokAt(2)->varId()) return true; + // std::string dereferences nullpointers + if (Token::Match(tok->tokAt(-4), "std :: string ( %var% )")) + return true; + if (Token::Match(tok->tokAt(-5), "std :: string %var% ( %var% )")) + return true; + + unsigned int ovarid = 0; + if (Token::Match(tok, "%var% ==|!= %var%")) + ovarid = tok->tokAt(2)->varId(); + else if (Token::Match(tok->tokAt(-2), "%var% ==|!=|=|+= %var%")) + ovarid = tok->tokAt(-2)->varId(); + if (ovarid) { + const Variable* var = symbolDatabase->getVariableFromVarId(ovarid); + if (var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "const| std :: string !!::")) + return true; + } + // Check if it's NOT a pointer dereference. // This is most useful in inconclusive checking if (inconclusive) { @@ -382,7 +399,7 @@ void CheckNullPointer::nullPointerAfterLoop() bool unknown = _settings->inconclusive; // Is the loop variable dereferenced? - if (CheckNullPointer::isPointerDeRef(tok2, unknown)) { + if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase)) { nullPointerError(tok2, varname, tok->linenr(), inconclusive); } @@ -422,31 +439,33 @@ void CheckNullPointer::nullPointerLinkedList() if (varid == 0) continue; + // Is this variable a pointer? + const Variable* var = symbolDatabase->getVariableFromVarId(varid); + if (!var || !var->isPointer()) + continue; + if (Token::Match(tok2->tokAt(-2), "%varid% ?", varid)) continue; - // Variable name of dereferenced variable - const std::string varname(tok2->str()); - // Check usage of dereferenced variable in the loop.. - for (const Token *tok3 = i->classStart->next(); tok3 && tok3 != i->classEnd; tok3 = tok3->next()) { + for (std::list::const_iterator j = i->nestedList.begin(); j != i->nestedList.end(); ++j) { + Scope* scope = *j; + if (scope->type != Scope::eWhile) + continue; + // TODO: are there false negatives for "while ( %varid% ||" - if (Token::Match(tok3, "while ( %varid% &&|)", varid)) { + if (Token::Match(scope->classDef->next(), "( %varid% &&|)", varid)) { // Make sure there is a "break" or "return" inside the loop. // Without the "break" a null pointer could be dereferenced in the // for statement. // indentlevel4 is a counter for { and }. When scanning the code with tok4 unsigned int indentlevel4 = 1; - for (const Token *tok4 = tok3->next()->link(); tok4; tok4 = tok4->next()) { + for (const Token *tok4 = scope->classStart; tok4; tok4 = tok4->next()) { if (tok4->str() == "{") ++indentlevel4; else if (tok4->str() == "}") { if (indentlevel4 <= 1) { - const Variable* var = symbolDatabase->getVariableFromVarId(varid); - // Is this variable a pointer? - if (var && var->isPointer()) - nullPointerError(tok1, varname, tok3->linenr()); - + nullPointerError(tok1, var->name(), scope->classDef->linenr()); break; } --indentlevel4; @@ -758,7 +777,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() // unknown : this is set by isPointerDeRef if it is // uncertain - bool unknown = false; + bool unknown = _settings->inconclusive; if (Token::Match(tok1->tokAt(-2), "%varid% = %varid% .", varid)) { break; @@ -772,7 +791,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() break; } else if (Token::Match(tok1->tokAt(-2), "&&|%oror% !")) { break; - } else if (CheckNullPointer::isPointerDeRef(tok1, unknown)) { + } else if (CheckNullPointer::isPointerDeRef(tok1, unknown, symbolDatabase)) { nullPointerError(tok1, varname, tok->linenr(), inconclusive); break; } else if (Token::simpleMatch(tok1->previous(), "&")) { @@ -891,16 +910,22 @@ void CheckNullPointer::nullPointerByCheckAndDeRef() } } - if (Token::Match(tok2, "goto|return|continue|break|throw|if|switch|for")) { - bool dummy = false; - if (Token::Match(tok2, "return * %varid%", varid)) - nullPointerError(tok2, pointerName, linenr, inconclusive); - else if (Token::Match(tok2, "return %varid%", varid) && - CheckNullPointer::isPointerDeRef(tok2->next(), dummy)) - nullPointerError(tok2, pointerName, linenr, inconclusive); + if (tok2->str() == "return" || tok2->str() == "throw") { + bool unknown = _settings->inconclusive; + for (; tok2 && tok2->str() != ";"; tok2 = tok2->next()) { + if (tok2->varId() == varid) { + if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase)) + nullPointerError(tok2, pointerName, linenr, inconclusive); + else if (unknown) + nullPointerError(tok2, pointerName, linenr, true); + } + } break; } + if (Token::Match(tok2, "goto|continue|break|if|switch|for")) + break; + // parameters to sizeof are not dereferenced if (Token::Match(tok2, "decltype|sizeof")) { if (tok2->strAt(1) != "(") @@ -949,7 +974,7 @@ void CheckNullPointer::nullPointerByCheckAndDeRef() if (Token::Match(tok2->previous(), "[;{}=] %var% = 0 ;")) ; - else if (CheckNullPointer::isPointerDeRef(tok2, unknown)) + else if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase)) nullPointerError(tok2, pointerName, linenr, inconclusive); else if (unknown && _settings->inconclusive) @@ -982,15 +1007,18 @@ void CheckNullPointer::nullConstantDereference() continue; for (const Token *tok = i->classStart; tok != i->classEnd; tok = tok->next()) { - if (tok->str() == "(" && Token::Match(tok->previous(), "sizeof|decltype|typeid")) - tok = tok->link(); + if (Token::Match(tok, "sizeof|decltype|typeid (")) + tok = tok->next()->link(); else if (Token::simpleMatch(tok, "* 0")) { - if (Token::Match(tok->previous(), "return|;|{|}|=|(|,|%op%")) { + if (Token::Match(tok->previous(), "return|throw|;|{|}|:|[|(|,") || tok->previous()->isOp() || tok->previous()->isAssignmentOp()) { nullPointerError(tok); } } + else if (Token::Match(tok, "0 [") && (tok->previous()->str() != "&" || !Token::Match(tok->next()->link()->next(), "[.(]"))) + nullPointerError(tok); + else if (Token::Match(tok->previous(), "[={};] %var% (")) { std::list var; parseFunctionCall(*tok, var, 0); @@ -1001,6 +1029,20 @@ void CheckNullPointer::nullConstantDereference() nullPointerError(*it); } } + } else if (Token::Match(tok, "std :: string ( 0 )")) + nullPointerError(tok); + if (Token::Match(tok, "std :: string %var% ( 0 )")) + nullPointerError(tok); + + unsigned int ovarid = 0; + if (Token::Match(tok, "0 ==|!= %var%")) + ovarid = tok->tokAt(2)->varId(); + else if (Token::Match(tok, "%var% ==|!=|=|+= 0")) + ovarid = tok->varId(); + if (ovarid) { + const Variable* var = symbolDatabase->getVariableFromVarId(ovarid); + if (var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "const| std :: string !!::")) + nullPointerError(tok); } } } @@ -1018,13 +1060,16 @@ void CheckNullPointer::nullConstantDereference() class Nullpointer : public ExecutionPath { public: /** Startup constructor */ - explicit Nullpointer(Check *c) : ExecutionPath(c, 0), null(false) { + Nullpointer(Check *c, const SymbolDatabase* symbolDatabase_) : ExecutionPath(c, 0), symbolDatabase(symbolDatabase_), null(false) { } private: + const SymbolDatabase* symbolDatabase; + /** Create checking of specific variable: */ - Nullpointer(Check *c, const unsigned int id, const std::string &name) + Nullpointer(Check *c, const unsigned int id, const std::string &name, const SymbolDatabase* symbolDatabase_) : ExecutionPath(c, id), + symbolDatabase(symbolDatabase_), varname(name), null(false) { } @@ -1082,56 +1127,22 @@ private: /** parse tokens */ const Token *parse(const Token &tok, std::list &checks) const { - if (Token::Match(tok.previous(), "[;{}] const| struct| %type% * %var% ;")) { - const Token * vartok = tok.tokAt(2); - - if (tok.str() == "const") - vartok = vartok->next(); - - if (tok.str() == "struct") - vartok = vartok->next(); - - if (vartok->varId() != 0) - checks.push_back(new Nullpointer(owner, vartok->varId(), vartok->str())); - return vartok->next(); - } - - // Template pointer variable.. - if (Token::Match(tok.previous(), "[;{}] %type% ::|<")) { - const Token * vartok = &tok; - while (Token::Match(vartok, "%type% ::")) - vartok = vartok->tokAt(2); - if (Token::Match(vartok, "%type% < %type%")) { - vartok = vartok->tokAt(3); - while (vartok && (vartok->str() == "*" || vartok->isName())) - vartok = vartok->next(); - } - if (vartok - && (vartok->str() == ">" || vartok->isName()) - && Token::Match(vartok->next(), "* %var% ;|=")) { - vartok = vartok->tokAt(2); - checks.push_back(new Nullpointer(owner, vartok->varId(), vartok->str())); - if (Token::simpleMatch(vartok->next(), "= 0 ;")) - setnull(checks, vartok->varId()); - return vartok->next(); - } + if (tok.varId() != 0) { + // Pointer declaration declaration? + const Variable* var = symbolDatabase->getVariableFromVarId(tok.varId()); + if (var && var->isPointer() && var->nameToken() == &tok) + checks.push_back(new Nullpointer(owner, var->varId(), var->name(), symbolDatabase)); } if (Token::simpleMatch(&tok, "try {")) { // Bail out all used variables - unsigned int indentlevel = 0; - for (const Token *tok2 = &tok; tok2; tok2 = tok2->next()) { - if (tok2->str() == "{") - ++indentlevel; - else if (tok2->str() == "}") { - if (indentlevel == 0) - break; - if (indentlevel == 1 && !Token::simpleMatch(tok2,"} catch (")) - return tok2; - --indentlevel; - } else if (tok2->varId()) + const Token* tok2 = &tok; + const Token* end = tok.linkAt(1); + for (; tok2 && tok2 != end; tok2 = tok2->next()) { + if (tok2->varId()) bailOutVar(checks,tok2->varId()); } + return tok2; } if (Token::Match(&tok, "%var% (")) { @@ -1149,21 +1160,21 @@ private: return tok.link(); if (tok.varId() != 0) { - // unknown : not really used. it is passed to isPointerDeRef. - // if isPointerDeRef fails to determine if there - // is a dereference the this will be set to true. + // unknown: if isPointerDeRef fails to determine if there + // is a dereference this will be set to true. bool unknown = owner->inconclusiveFlag(); + bool deref = CheckNullPointer::isPointerDeRef(&tok, unknown, symbolDatabase); - if (Token::Match(tok.previous(), "[;{}=] %var% = 0 ;")) - setnull(checks, tok.varId()); - else if (CheckNullPointer::isPointerDeRef(&tok, unknown)) + if (deref) dereference(checks, &tok); else if (unknown && owner->inconclusiveFlag()) dereference(checks, &tok); - else - // TODO: Report debug warning that it's unknown if a - // pointer is dereferenced - bailOutVar(checks, tok.varId()); + if (Token::Match(tok.previous(), "[;{}=] %var% = 0 ;")) + setnull(checks, tok.varId()); + else if (!deref && + !tok.previous()->isOp() && !tok.previous()->isAssignmentOp() && + (!tok.next()->isOp() || tok.next()->str() == ">>")) + bailOutVar(checks, tok.varId()); // If its possible that the pointers value changes, bail out. } else if (tok.str() == "delete") { @@ -1175,13 +1186,15 @@ private: } else if (tok.str() == "return") { - bool unknown = false; - const Token *vartok = tok.next(); - if (vartok->str() == "*") - vartok = vartok->next(); - if (vartok->varId() && CheckNullPointer::isPointerDeRef(vartok, unknown)) { - dereference(checks, vartok); + bool unknown = owner->inconclusiveFlag(); + const Token* tok2 = &tok; + for (; tok2 && tok2->str() != ";"; tok2 = tok2->next()) { + if (tok2->varId()) { + if (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase) || unknown) + dereference(checks, tok2); + } } + return tok2; } return &tok; @@ -1192,8 +1205,9 @@ private: for (const Token *tok2 = &tok; tok2; tok2 = tok2->next()) { if (tok2->str() == "(" || tok2->str() == ")") break; - if (Token::Match(tok2, "[<>=] * %var%")) - dereference(checks, tok2->tokAt(2)); + bool unknown = owner->inconclusiveFlag(); + if (tok2->varId() && (CheckNullPointer::isPointerDeRef(tok2, unknown, symbolDatabase) || unknown)) + dereference(checks, tok2); } if (Token::Match(&tok, "!| %var% (")) { @@ -1224,7 +1238,7 @@ private: void CheckNullPointer::executionPaths() { // Check for null pointer errors.. - Nullpointer c(this); + Nullpointer c(this, _tokenizer->getSymbolDatabase()); checkExecutionPaths(_tokenizer->tokens(), &c); } @@ -1246,6 +1260,3 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var else reportError(tok, Severity::error, "nullPointer", errmsg); } - - - diff --git a/lib/checknullpointer.h b/lib/checknullpointer.h index 0c6abe548..72e2f3051 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -26,6 +26,7 @@ #include "settings.h" class Token; +class SymbolDatabase; /// @addtogroup Checks /// @{ @@ -80,7 +81,7 @@ public: * @param unknown it is not known if there is a pointer dereference (could be reported as a debug message) * @return true => there is a dereference */ - static bool isPointerDeRef(const Token *tok, bool &unknown); + static bool isPointerDeRef(const Token *tok, bool &unknown, const SymbolDatabase* symbolDatabase); /** @brief possible null pointer dereference */ void nullPointer(); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 4db842068..d92620e5e 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1262,7 +1262,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer) const } bool unknown = false; - if (pointer && CheckNullPointer::isPointerDeRef(vartok, unknown)) { + if (pointer && CheckNullPointer::isPointerDeRef(vartok, unknown, _tokenizer->getSymbolDatabase())) { // function parameter? bool functionParameter = false; if (Token::Match(vartok->tokAt(-2), "%var% (") || vartok->previous()->str() == ",") diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 4d100cd83..5bac3872c 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -60,6 +60,7 @@ private: TEST_CASE(nullpointer_in_for_loop); TEST_CASE(nullpointerDelete); TEST_CASE(nullpointerExit); + TEST_CASE(nullpointerStdString); TEST_CASE(functioncall); } @@ -1283,6 +1284,15 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); + check("int foo(int *p) {\n" + " if (!p) {\n" + " x = *p;\n" + " return 5+*p;\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Possible null pointer dereference: p - otherwise it is redundant to check if p is null at line 2\n" + "[test.cpp:4]: (error) Possible null pointer dereference: p - otherwise it is redundant to check if p is null at line 2\n", errout.str()); + // operator! check("void f() {\n" " A a;\n" @@ -1486,6 +1496,12 @@ private: " image1.fseek(0, SEEK_SET);\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int* p = 0;\n" + " return p[4];\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n", errout.str()); } void gcc_statement_expression() { @@ -1719,6 +1735,42 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointerStdString() { + check("void f(std::string s1) {\n" + " void* p = 0;\n" + " s1 = 0;\n" + " std::string s2 = 0;\n" + " std::string s3(0);\n" + " foo(std::string(0));\n" + " s1 = p;\n" + " std::string s4 = p;\n" + " std::string s5(p);\n" + " foo(std::string(p));\n" + "}", true); + ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n" + "[test.cpp:4]: (error) Null pointer dereference\n" + "[test.cpp:5]: (error) Null pointer dereference\n" + "[test.cpp:6]: (error) Null pointer dereference\n" + "[test.cpp:7]: (error) Possible null pointer dereference: p\n" + "[test.cpp:8]: (error) Possible null pointer dereference: p\n" + "[test.cpp:9]: (error) Possible null pointer dereference: p\n" + "[test.cpp:10]: (error) Possible null pointer dereference: p\n", errout.str()); + + check("void f(std::string s1, const std::string& s2, const std::string* s3) {\n" + " void* p = 0;\n" + " foo(s1 == p);\n" + " foo(s2 == p);\n" + " foo(s3 == p);\n" + " foo(p == s1);\n" + " foo(p == s2);\n" + " foo(p == s3);\n" + "}", true); + ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference\n" + "[test.cpp:4]: (error) Possible null pointer dereference: p\n" + "[test.cpp:6]: (error) Possible null pointer dereference: p\n" + "[test.cpp:7]: (error) Possible null pointer dereference: p\n", errout.str()); + } + void functioncall() { // #3443 - function calls // dereference pointer and then check if it's null {