From 9dd4ac68c0f47c6bcdce9753c176e3feff1a6359 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Thu, 22 May 2014 19:48:00 +0200 Subject: [PATCH] Refactorization in CheckNullPointer: - Use AST to detect dereferences - Added more unit tests - Removed handling of unknown constructs in CheckNullPointer::isPointerDeRef() Added link to verbose message cstyleCast. --- lib/checknullpointer.cpp | 112 +++++++++++++-------------------------- lib/checkother.cpp | 2 +- test/testnullpointer.cpp | 30 +++++++++-- 3 files changed, 64 insertions(+), 80 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 542520eeb..942e9422f 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -327,35 +327,34 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) "stringstream", "wistringstream", "wostringstream", "wstringstream" }; - const bool inconclusive = unknown; - unknown = false; - const Token* prev = tok->previous(); - while (prev->str() == "." || prev->str() == "::") { // Skip over previous member dereferences - prev = prev->previous(); - while (prev->link() && (prev->str() == "]" || prev->str() == ">")) - prev = prev->link()->previous(); - prev = prev->previous(); + const Token* parent = tok->astParent(); + if (!parent) + return false; + bool firstOperand = parent->astOperand1() == tok; + while (parent->str() == "(" && (parent->astOperand2() == nullptr && parent->strAt(1) != ")")) { // Skip over casts + parent = parent->astParent(); + if (!parent) + return false; } - while (prev->str() == ")") // Skip over casts - prev = prev->link()->previous(); // Dereferencing pointer.. - if (prev->str() == "*" && (Token::Match(prev->previous(), "return|throw|;|{|}|:|[|(|,") || prev->previous()->isOp()) && !Token::Match(prev->tokAt(-2), "sizeof|decltype|typeof")) + if (parent->str() == "*" && !parent->astOperand2() && !Token::Match(parent->tokAt(-2), "sizeof|decltype|typeof")) + return true; + + // array access + if (parent->str() == "[" && (!parent->astParent() || parent->astParent()->str() != "&")) return true; // read/write member variable - if (!Token::simpleMatch(prev->previous(), "& (") && !Token::Match(prev->previous(), "sizeof|decltype|typeof (") && prev->str() != "&" && Token::Match(tok->next(), ". %var%")) { - if (tok->strAt(3) != "(") + if (firstOperand && parent->str() == "." && (!parent->astParent() || parent->astParent()->str() != "&")) { + if (!parent->astParent() || parent->astParent()->str() != "(" || parent->astParent() == tok->previous()) return true; unknown = true; return false; } - if (Token::Match(tok, "%var% [") && (prev->str() != "&" || Token::Match(tok->next()->link()->next(), "[.(]"))) - return true; - if (Token::Match(tok, "%var% (")) return true; @@ -365,19 +364,19 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) return true; // std::string dereferences nullpointers - if (Token::Match(prev->tokAt(-3), "std :: string|wstring (") && tok->strAt(1) == ")") + if (Token::Match(parent->tokAt(-3), "std :: string|wstring (") && tok->strAt(1) == ")") return true; - if (Token::Match(prev->previous(), "%var% (") && tok->strAt(1) == ")") { + if (Token::Match(parent->previous(), "%var% (") && tok->strAt(1) == ")") { const Variable* var = tok->tokAt(-2)->variable(); if (var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "std :: string|wstring !!::")) return true; } // streams dereference nullpointers - if (Token::Match(prev, "<<|>>")) { + if (Token::Match(parent, "<<|>>") && !firstOperand) { const Variable* var = tok->variable(); if (var && var->isPointer() && Token::Match(var->typeStartToken(), "char|wchar_t")) { // Only outputting or reading to char* can cause problems - const Token* tok2 = prev; // Find start of statement + const Token* tok2 = parent; // Find start of statement for (; tok2; tok2 = tok2->previous()) { if (Token::Match(tok2->previous(), ";|{|}|:")) break; @@ -393,64 +392,15 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) } const Variable *ovar = nullptr; - if (Token::Match(tok, "%var% ==|!= %var%")) - ovar = tok->tokAt(2)->variable(); - else if (Token::Match(prev->previous(), "%var% ==|!=")) - ovar = tok->tokAt(-2)->variable(); - else if (Token::Match(prev->previous(), "%var% =|+") && Token::Match(tok->next(), ")|]|,|;|+")) - ovar = tok->tokAt(-2)->variable(); + if (Token::Match(parent, "+|==|!=") || (parent->str() == "=" && !firstOperand)) { + if (parent->astOperand1() == tok && parent->astOperand2()) + ovar = parent->astOperand2()->variable(); + else if (parent->astOperand2() == tok) + ovar = parent->astOperand1()->variable(); + } if (ovar && !ovar->isPointer() && !ovar->isArray() && Token::Match(ovar->typeStartToken(), "std :: string|wstring !!::")) return true; - // Check if it's NOT a pointer dereference. - // This is most useful in inconclusive checking - if (inconclusive) { - // Not a dereference.. - if (Token::Match(tok, "%var% =")) - return false; - - // OK to delete a null - if (Token::simpleMatch(prev, "delete") || Token::Match(prev->tokAt(-2), "delete [ ]")) - return false; - - // OK to check if pointer is null - // OK to take address of pointer - if (Token::Match(prev, "!|&")) - return false; - - // OK to check pointer in "= p ? : " - if (tok->next()->str() == "?" && - (Token::Match(prev, "return|throw|;|{|}|:|[|(|,") || prev->isAssignmentOp())) - return false; - - // OK to pass pointer to function - if (Token::Match(prev, "[(,]") && Token::Match(tok->next(), "[,)]") && - (prev->str() != "(" || - Token::Match(prev->previous(), "%var% ("))) - return false; - - // Compare pointer - if (Token::Match(prev, "(|&&|%oror%|==|!=")) - return false; - if (Token::Match(tok, "%var% &&|%oror%|==|!=|)")) - return false; - - // Taking address - if (Token::Match(prev, "return|=") && tok->strAt(1) == ";") - return false; - - // (void)var - if (Token::Match(prev, "[{;}]") && tok->strAt(1) == ";") - return false; - - // Shift pointer (e.g. to cout, but its no char* (see above)) - if (Token::Match(prev, "<<|>>")) - return false; - - // unknown if it's a dereference - unknown = true; - } - // assume that it's not a dereference (no false positives) return false; } @@ -962,7 +912,17 @@ void CheckNullPointer::nullPointerDefaultArgument() for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { // If we encounter a possible NULL-pointer check, skip over its body - if (Token::simpleMatch(tok, "if ( ")) { + if (tok->str() == "?") { // TODO: Skip this if the condition is unrelated to the variables + // Find end of statement + tok = tok->astOperand2(); + while (tok && !Token::Match(tok, ")|;")) { + if (tok->link() && Token::Match(tok, "(|[|<|{")) + tok = tok->link(); + tok = tok->next(); + } + if (!tok) + break; + } else if (Token::simpleMatch(tok, "if ( ")) { bool dependsOnPointer = false; const Token *endOfCondition = tok->next()->link(); if (!endOfCondition) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 3f8e6c36b..254b4dd9c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -516,7 +516,7 @@ void CheckOther::cstyleCastError(const Token *tok) "C-style pointer casting detected. C++ offers four different kinds of casts as replacements: " "static_cast, const_cast, dynamic_cast and reinterpret_cast. A C-style cast could evaluate to " "any of those automatically, thus it is considered safer if the programmer explicitly states " - "which kind of cast is expected."); + "which kind of cast is expected. See also: https://www.securecoding.cert.org/confluence/display/cplusplus/EXP05-CPP.+Do+not+use+C-style+casts."); } //--------------------------------------------------------------------------- diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 892ed756f..0606bbbe0 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -295,10 +295,14 @@ private: check("void foo(struct ABC *abc) {\n" " bar(abc->a);\n" + " bar(x, abc->a);\n" + " bar(x, y, abc->a);\n" " if (!abc)\n" " ;\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Possible null pointer dereference: abc - otherwise it is redundant to check it against null.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:5]: (warning) Possible null pointer dereference: abc - otherwise it is redundant to check it against null.\n" + "[test.cpp:3] -> [test.cpp:5]: (warning) Possible null pointer dereference: abc - otherwise it is redundant to check it against null.\n" + "[test.cpp:4] -> [test.cpp:5]: (warning) Possible null pointer dereference: abc - otherwise it is redundant to check it against null.\n", errout.str()); check("void foo(ABC *abc) {\n" " if (abc->a == 3) {\n" @@ -619,7 +623,7 @@ private: " if (!p)\n" " ;\n" "}"); - TODO_ASSERT_EQUALS("error", "", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Possible null pointer dereference: p - otherwise it is redundant to check it against null.\n", errout.str()); // while check("void f(int *p) {\n" @@ -2377,7 +2381,22 @@ private: ASSERT_EQUALS("", errout.str()); check("void f(int *p = 0) {\n" - " std::cout << p ? *p : 0;\n" + " std::cout << p ? *p : 0;\n" // Due to operator precedence, this is equivalent to: (std::cout << p) ? *p : 0; + "}"); + TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str()); // Check the first branch of ternary + + check("void f(char *p = 0) {\n" + " std::cout << p ? *p : 0;\n" // Due to operator precedence, this is equivalent to: (std::cout << p) ? *p : 0; + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str()); + + check("void f(int *p = 0) {\n" + " std::cout << (p ? *p : 0);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int *p = 0) {\n" + " std::cout << p;\n" "}"); ASSERT_EQUALS("", errout.str()); @@ -2442,6 +2461,11 @@ private: " *p = 0;\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void foo(int *p = 0) {\n" + " int var1 = x ? *p : 5;\n" + "}"); + TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str()); }