From b08f99a0820447d2eafc5e410246ed30b40b4a2b Mon Sep 17 00:00:00 2001 From: PKEuS Date: Fri, 24 Feb 2017 19:10:34 +0100 Subject: [PATCH] Fixed false negative: nullpointer passed as std::string argument (#7927) Refactorization: Removed dead code from CheckNullPointer::parseFunctionCall() --- lib/checknullpointer.cpp | 31 ++++++++++------------ lib/checknullpointer.h | 5 +--- test/testnullpointer.cpp | 56 ++++++++++------------------------------ 3 files changed, 28 insertions(+), 64 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 7111ba43c..919ce232d 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -48,7 +48,7 @@ static bool checkNullpointerFunctionCallPlausibility(const Function* func, unsig * @param value 0 => invalid with null pointers as parameter. * 1-.. => only invalid with uninitialized data. */ -void CheckNullPointer::parseFunctionCall(const Token &tok, std::list &var, const Library *library, unsigned char value) +void CheckNullPointer::parseFunctionCall(const Token &tok, std::list &var, const Library *library) { if (Token::Match(&tok, "%name% ( )") || !tok.tokAt(2)) return; @@ -57,21 +57,18 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::listnextArgument(); // 1st parameter.. - if (Token::Match(firstParam, "%var% ,|)") || - (value == 0 && Token::Match(firstParam, "0|NULL ,|)"))) { - if (value == 0 && Token::Match(&tok, "snprintf|vsnprintf|fnprintf|vfnprintf") && secondParam && secondParam->str() != "0") // Only if length (second parameter) is not zero - var.push_back(firstParam); - } + if (Token::Match(&tok, "snprintf|vsnprintf|fnprintf|vfnprintf") && secondParam && secondParam->str() != "0") // Only if length (second parameter) is not zero + var.push_back(firstParam); - // Library - if (library) { + if (library || tok.function() != nullptr) { const Token *param = firstParam; int argnr = 1; while (param) { - if (Token::Match(param, "%var% ,|)") || (value==0 && Token::Match(param, "0|NULL ,|)"))) { - if (value == 0 && library->isnullargbad(&tok, argnr) && checkNullpointerFunctionCallPlausibility(tok.function(), argnr)) - var.push_back(param); - else if (value == 1 && library->isuninitargbad(&tok, argnr)) + if (library && library->isnullargbad(&tok, argnr) && checkNullpointerFunctionCallPlausibility(tok.function(), argnr)) + var.push_back(param); + else if (tok.function()) { + const Variable* argVar = tok.function()->getArgumentVar(argnr-1); + if (argVar && argVar->isStlStringType() && !argVar->isArrayOrPointer()) var.push_back(param); } param = param->nextArgument(); @@ -124,10 +121,8 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::liststr() == "0") || Token::Match(argListTok, "%var% [,)]")) { - var.push_back(argListTok); - } + if ((*i == 'n' || *i == 's' || scan)) { + var.push_back(argListTok); } if (*i != 'm') // %m is a non-standard glibc extension that requires no parameter @@ -345,7 +340,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() if (!ftok || !ftok->previous()) continue; std::list varlist; - parseFunctionCall(*ftok->previous(), varlist, &_settings->library, 0); + parseFunctionCall(*ftok->previous(), varlist, &_settings->library); if (std::find(varlist.begin(), varlist.end(), tok) != varlist.end()) { if (value->condition == nullptr) nullPointerError(tok, tok->str(), false, value->defaultArg, !value->isKnown()); @@ -422,7 +417,7 @@ void CheckNullPointer::nullConstantDereference() nullPointerError(tok); } else { // function call std::list var; - parseFunctionCall(*tok, var, &_settings->library, 0); + parseFunctionCall(*tok, var, &_settings->library); // is one of the var items a NULL pointer? for (std::list::const_iterator it = var.begin(); it != var.end(); ++it) { diff --git a/lib/checknullpointer.h b/lib/checknullpointer.h index 2c6c528a5..cd6c7e70f 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -61,13 +61,10 @@ public: * @param tok first token * @param var variables that the function read / write. * @param library --library files data - * @param value 0 => invalid with null pointers as parameter. - * non-zero => invalid with uninitialized data. */ static void parseFunctionCall(const Token &tok, std::list &var, - const Library *library, - unsigned char value); + const Library *library); /** * Is there a pointer dereference? Everything that should result in diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index ffa510d79..50a3126a5 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -2075,6 +2075,16 @@ private: " ASSERT_MESSAGE(\"Error on s\", 0 == s.compare(\"Some text\"));\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void foo(int i, std::string s);\n" + "void bar() {\n" + " foo(0, \"\");\n" + " foo(0, 0);\n" + " foo(var, 0);\n" + " foo(0, var);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference\n" + "[test.cpp:5]: (error) Null pointer dereference\n", errout.str()); } void nullpointerStdStream() { @@ -2258,11 +2268,9 @@ private: library.functions["x"].argumentChecks[2] = arg; library.functions["x"].argumentChecks[3] = arg; - std::list null, uninit; - CheckNullPointer::parseFunctionCall(*xtok, null, &library, 0U); - CheckNullPointer::parseFunctionCall(*xtok, uninit, &library, 1U); + std::list null; + CheckNullPointer::parseFunctionCall(*xtok, null, &library); ASSERT_EQUALS(0U, null.size()); - ASSERT_EQUALS(0U, uninit.size()); } // for 1st parameter null pointer is not ok.. @@ -2274,46 +2282,10 @@ private: library.functions["x"].argumentChecks[3] = arg; library.functions["x"].argumentChecks[1].notnull = true; - std::list null,uninit; - CheckNullPointer::parseFunctionCall(*xtok, null, &library, 0U); - CheckNullPointer::parseFunctionCall(*xtok, uninit, &library, 1U); + std::list null; + CheckNullPointer::parseFunctionCall(*xtok, null, &library); ASSERT_EQUALS(1U, null.size()); ASSERT_EQUALS("a", null.front()->str()); - ASSERT_EQUALS(0U, uninit.size()); - } - - // for 2nd parameter uninit data is not ok.. - { - Library library; - Library::ArgumentChecks arg; - library.functions["x"].argumentChecks[1] = arg; - library.functions["x"].argumentChecks[2] = arg; - library.functions["x"].argumentChecks[3] = arg; - library.functions["x"].argumentChecks[2].notuninit = true; - - std::list null,uninit; - CheckNullPointer::parseFunctionCall(*xtok, null, &library, 0U); - CheckNullPointer::parseFunctionCall(*xtok, uninit, &library, 1U); - ASSERT_EQUALS(0U, null.size()); - ASSERT_EQUALS(1U, uninit.size()); - ASSERT_EQUALS("b", uninit.front()->str()); - } - - // for 3rd parameter uninit data is not ok.. - { - Library library; - Library::ArgumentChecks arg; - library.functions["x"].argumentChecks[1] = arg; - library.functions["x"].argumentChecks[2] = arg; - library.functions["x"].argumentChecks[3] = arg; - library.functions["x"].argumentChecks[3].notuninit = true; - - std::list null,uninit; - CheckNullPointer::parseFunctionCall(*xtok, null, &library, 0U); - CheckNullPointer::parseFunctionCall(*xtok, uninit, &library, 1U); - ASSERT_EQUALS(0U, null.size()); - ASSERT_EQUALS(1U, uninit.size()); - ASSERT_EQUALS("c", uninit.front()->str()); } }