From f1dbd1a89a359670c8e802db712823dfbd0008ef Mon Sep 17 00:00:00 2001 From: Erik Lax Date: Sat, 11 Feb 2012 16:15:38 +0100 Subject: [PATCH] Fixed #3518 (False negative: Possible null pointer dereference (in the same condition)) --- AUTHORS | 1 + lib/checknullpointer.cpp | 43 +++++++++++++++++++++++++------- test/testnullpointer.cpp | 53 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 9 deletions(-) diff --git a/AUTHORS b/AUTHORS index be057262f..9dcd22828 100644 --- a/AUTHORS +++ b/AUTHORS @@ -3,6 +3,7 @@ The cppcheck team, in alphabetical order: Bill Egert Daniel Marjamäki Edoardo Prezioso +Erik Lax Gianluca Scacco Greg Hewgill Hoang Tuan Su diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 04112a264..b234eca4e 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -839,13 +839,17 @@ void CheckNullPointer::nullPointerByCheckAndDeRef() // vartok : token for the variable const Token *vartok = 0; - if (Token::Match(tok, "( ! %var% )|&&")) + const Token *checkConditionStart = 0; + if (Token::Match(tok, "( ! %var% )|&&")) { vartok = tok->tokAt(2); - else if (Token::Match(tok, "( %var% )|&&")) + checkConditionStart = vartok->next(); + } else if (Token::Match(tok, "( %var% )|&&")) { vartok = tok->next(); - else if (Token::Match(tok, "( ! ( %var% =")) + } else if (Token::Match(tok, "( ! ( %var% =")) { vartok = tok->tokAt(3); - else + if (Token::Match(tok->tokAt(2)->link(), ") &&")) + checkConditionStart = tok->tokAt(2)->link(); + } else continue; // variable id for pointer @@ -853,17 +857,42 @@ void CheckNullPointer::nullPointerByCheckAndDeRef() if (varid == 0) continue; - const Variable* var = _tokenizer->getSymbolDatabase()->getVariableFromVarId(varid); // Check if variable is a pointer + const Variable* var = _tokenizer->getSymbolDatabase()->getVariableFromVarId(varid); if (!var || !var->isPointer()) continue; if (Token::Match(vartok->next(), "&& ( %varid% =", varid)) continue; + // Name and line of the pointer + const std::string &pointerName = vartok->str(); + const unsigned int linenr = vartok->linenr(); + // if this is true then it is known that the pointer is null bool null = true; + // Check the condition (eg. ( !x && x->i ) + if (checkConditionStart) { + const Token * const conditionEnd = tok->link(); + for (const Token *tok2 = checkConditionStart; tok2 != conditionEnd; tok2 = tok2->next()) { + // If we hit a || operator, abort + if (Token::Match(tok2, "%oror%", varid)) { + break; + } + // Pointer is used + else if (Token::Match(tok2, "* %varid%", varid)) { + nullPointerError(tok2->tokAt(2), pointerName, linenr, false); + break; + } + // Pointer is used + else if (Token::Match(tok2, "%varid% .", varid)) { + nullPointerError(tok2, pointerName, linenr, false); + break; + } + } + } + // start token = inside the if-body const Token *tok1 = i->classStart; @@ -877,10 +906,6 @@ void CheckNullPointer::nullPointerByCheckAndDeRef() continue; } - // Name and line of the pointer - const std::string &pointerName = vartok->str(); - const unsigned int linenr = vartok->linenr(); - unsigned int indentlevel = 0; // Set to true if we would normally bail out the check. diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 64acd1a87..325296014 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -1478,6 +1478,59 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + // check, assign and use + check("void f() {\n" + " char *p;\n" + " if (p == 0 && (p = malloc(10)) != a && (*p = a)) {\n" + " *p = 0;\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // check, and use + check("void f() {\n" + " char *p;\n" + " if (p == 0 && (*p = 0)) {\n" + " return;\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 3\n", errout.str()); + + // check, and use + check("void f() {\n" + " struct foo *p;\n" + " if (p == 0 && p->x == 10) {\n" + " return;\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 3\n", errout.str()); + + // check, and use + check("void f() {\n" + " struct foo *p;\n" + " if (p == 0 || p->x == 10) {\n" + " return;\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // check, and use + check("void f() {\n" + " char *p;\n" + " if ((p = malloc(10)) == NULL && (*p = a)) {\n" + " return;\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 3\n", errout.str()); + + // check, and use + check("void f(struct X *p, int x) {\n" + " if (!p && x==1 || p && p->x==0) {\n" + " return;\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + { const char code[] = "void f(Fred *fred) {\n" " if (fred == NULL) { }\n"