diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 5fd23e297..2c9d8f1c9 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -327,7 +327,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() parseFunctionCall(*ftok->previous(), varlist, &_settings->library, 0); if (std::find(varlist.begin(), varlist.end(), tok) != varlist.end()) { if (value->condition == nullptr) - nullPointerError(tok, tok->str()); + nullPointerError(tok, tok->str(), false, value->defaultArg); else if (_settings->isEnabled("warning")) nullPointerError(tok, tok->str(), value->condition, value->inconclusive); } @@ -339,7 +339,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() if (!isPointerDeRef(tok,unknown)) { if (_settings->inconclusive && unknown) { if (value->condition == nullptr) - nullPointerError(tok, tok->str(), true); + nullPointerError(tok, tok->str(), true, value->defaultArg); else nullPointerError(tok, tok->str(), value->condition, true); } @@ -347,7 +347,7 @@ void CheckNullPointer::nullPointerByDeRefAndChec() } if (value->condition == nullptr) - nullPointerError(tok, tok->str(), value->inconclusive); + nullPointerError(tok, tok->str(), value->inconclusive, value->defaultArg); else if (_settings->isEnabled("warning")) nullPointerError(tok, tok->str(), value->condition, value->inconclusive); } @@ -357,7 +357,6 @@ void CheckNullPointer::nullPointer() { nullPointerLinkedList(); nullPointerByDeRefAndChec(); - nullPointerDefaultArgument(); } /** Dereferencing null constant (simplified token list) */ @@ -444,159 +443,18 @@ void CheckNullPointer::nullConstantDereference() } } -/** -* @brief If tok is a function call that passes in a pointer such that -* the pointer may be modified, this function will remove that -* pointer from pointerArgs. -*/ -void CheckNullPointer::removeAssignedVarFromSet(const Token* tok, std::set& pointerArgs) -{ - // If a pointer's address is passed into a function, stop considering it - if (Token::Match(tok->previous(), "[;{}] %name% (")) { - const Token* endParen = tok->next()->link(); - for (const Token* tok2 = tok->next(); tok2 != endParen; tok2 = tok2->next()) { - if (tok2->isName() && tok2->varId() > 0 - // Common functions that are known NOT to modify their pointer argument - && !Token::Match(tok, "printf|sprintf|fprintf|vprintf")) { - pointerArgs.erase(tok2->varId()); - } - } - } -} - -/** -* @brief Does one part of the check for nullPointer(). -* -# default argument that sets a pointer to 0 -* -# dereference pointer -*/ -void CheckNullPointer::nullPointerDefaultArgument() -{ - if (!_settings->isEnabled("warning")) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - const std::size_t functions = symbolDatabase->functionScopes.size(); - for (std::size_t i = 0; i < functions; ++i) { - const Scope * scope = symbolDatabase->functionScopes[i]; - if (scope->function == 0 || !scope->function->hasBody()) // We only look for functions with a body - continue; - - // Scan the argument list for default arguments that are pointers and - // which default to a NULL pointer if no argument is specified. - std::set pointerArgs; - for (const Token *tok = scope->function->arg; tok != scope->function->arg->link(); tok = tok->next()) { - - if (Token::Match(tok, "%var% = 0 ,|)")) { - const Variable *var = tok->variable(); - if (var && var->isPointer()) - pointerArgs.insert(tok->varId()); - } - } - - // Report an error if any of the default-NULL arguments are dereferenced - if (!pointerArgs.empty()) { - 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 (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) - continue; - - const Token *startOfIfBlock = - Token::simpleMatch(endOfCondition, ") {") ? endOfCondition->next() : nullptr; - if (!startOfIfBlock) - continue; - - // If this if() statement may return, it may be a null - // pointer check for the pointers referenced in its condition - const Token *endOfIf = startOfIfBlock->link(); - bool isExitOrReturn = - Token::findmatch(startOfIfBlock, "exit|return|throw", endOfIf) != nullptr; - - if (Token::Match(tok, "if ( %var% == 0 )")) { - const unsigned int varid = tok->tokAt(2)->varId(); - if (pointerArgs.count(varid) > 0) { - if (isExitOrReturn) - pointerArgs.erase(varid); - else - dependsOnPointer = true; - } - } else { - for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) { - if (tok2->isName() && tok2->varId() > 0 && - pointerArgs.count(tok2->varId()) > 0) { - - // If the if() depends on a pointer and may return, stop - // considering that pointer because it may be a NULL-pointer - // check that returns if the pointer is NULL. - if (isExitOrReturn) - pointerArgs.erase(tok2->varId()); - else - dependsOnPointer = true; - } - } - } - - if (dependsOnPointer && endOfIf) { - for (; tok != endOfIf; tok = tok->next()) { - // If a pointer is assigned a new value, stop considering it. - if (Token::Match(tok, "%var% =")) - pointerArgs.erase(tok->varId()); - else - removeAssignedVarFromSet(tok, pointerArgs); - } - continue; - } - } - - // If there is a noreturn function (e.g. exit()), stop considering the rest of - // this function. - bool unknown = false; - if (Token::Match(tok, "return|throw|exit") || - (_tokenizer->IsScopeNoReturn(tok, &unknown) && !unknown)) - break; - - removeAssignedVarFromSet(tok, pointerArgs); - - if (tok->varId() == 0 || pointerArgs.count(tok->varId()) == 0) - continue; - - // If a pointer is assigned a new value, stop considering it. - if (Token::Match(tok, "%var% =")) - pointerArgs.erase(tok->varId()); - - // If a pointer dereference is preceded by an && or ||, - // they serve as a sequence point so the dereference - // may not be executed. - if (isPointerDeRef(tok, unknown) && !unknown && - tok->strAt(-1) != "&&" && tok->strAt(-1) != "||" && - tok->strAt(-2) != "&&" && tok->strAt(-2) != "||") - nullPointerDefaultArgError(tok, tok->str()); - } - } - } -} - void CheckNullPointer::nullPointerError(const Token *tok) { reportError(tok, Severity::error, "nullPointer", "Null pointer dereference"); } -void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, bool inconclusive) +void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, bool inconclusive, bool defaultArg) { - reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname, inconclusive); + if (defaultArg) { + if (_settings->isEnabled("warning")) + reportError(tok, Severity::warning, "nullPointer", "Possible null pointer dereference if the default parameter value is used: " + varname, inconclusive); + } else + reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname, inconclusive); } void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, const Token* nullCheck, bool inconclusive) @@ -607,8 +465,3 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var const std::string errmsg("Possible null pointer dereference: " + varname + " - otherwise it is redundant to check it against null."); reportError(callstack, Severity::warning, "nullPointer", errmsg, inconclusive); } - -void CheckNullPointer::nullPointerDefaultArgError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, "nullPointer", "Possible null pointer dereference if the default parameter value is used: " + varname); -} diff --git a/lib/checknullpointer.h b/lib/checknullpointer.h index 65387b126..271c67db6 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -86,9 +86,8 @@ public: void nullConstantDereference(); void nullPointerError(const Token *tok); // variable name unknown / doesn't exist - void nullPointerError(const Token *tok, const std::string &varname, bool inconclusive=false); + void nullPointerError(const Token *tok, const std::string &varname, bool inconclusive = false, bool defaultArg = false); void nullPointerError(const Token *tok, const std::string &varname, const Token* nullcheck, bool inconclusive = false); - void nullPointerDefaultArgError(const Token *tok, const std::string &varname); private: /** Get error messages. Used by --errorlist */ @@ -119,26 +118,6 @@ private: * Dereferencing a pointer and then checking if it's NULL.. */ void nullPointerByDeRefAndChec(); - - /** - * @brief Does one part of the check for nullPointer(). - * -# initialize pointer to 0 - * -# conditionally assign pointer - * -# dereference pointer - */ - void nullPointerConditionalAssignment(); - - /** - * @brief Does one part of the check for nullPointer(). - * -# default argument that sets a pointer to 0 - * -# dereference pointer - */ - void nullPointerDefaultArgument(); - - /** - * @brief Removes any variable that may be assigned from pointerArgs. - */ - static void removeAssignedVarFromSet(const Token* tok, std::set& pointerArgs); }; /// @} //--------------------------------------------------------------------------- diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 21dee71c5..b6e80a50d 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -757,6 +757,15 @@ static bool valueFlowForward(Token * const startToken, return false; } + // Set values in condition + for (Token* tok3 = tok2->tokAt(2); tok3 != tok2->next()->link(); tok3 = tok3->next()) { + if (tok3->varId() == varid) { + for (std::list::const_iterator it = values.begin(); it != values.end(); ++it) + setTokenValue(tok3, *it); + break; + } + } + // Should scope be skipped because variable value is checked? std::list truevalues; for (std::list::iterator it = values.begin(); it != values.end(); ++it) { @@ -1560,6 +1569,20 @@ static void valueFlowForLoop(TokenList *tokenlist, SymbolDatabase* symboldatabas } } +static void valueFlowInjectParameter(TokenList* tokenlist, ErrorLogger* errorLogger, const Settings* settings, const Variable* arg, const Scope* functionScope, const std::list& argvalues) +{ + // Is argument passed by value or const reference, and is it a known non-class type? + if (arg->isReference() && !arg->isConst() && !arg->isClass()) + return; + + // Set value in function scope.. + const unsigned int varid2 = arg->declarationId(); + if (!varid2) + return; + + valueFlowForward(const_cast(functionScope->classStart->next()), functionScope->classEnd, arg, varid2, argvalues, true, tokenlist, errorLogger, settings); +} + static void valueFlowSubFunction(TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { @@ -1603,23 +1626,27 @@ static void valueFlowSubFunction(TokenList *tokenlist, ErrorLogger *errorLogger, continue; } } + valueFlowInjectParameter(tokenlist, errorLogger, settings, arg, functionScope, argvalues); + } + } +} - // Is argument passed by value? - if (!Token::Match(arg->typeStartToken(), "%type% %var% ,|)") && - !Token::Match(arg->typeStartToken(), "const| struct| %type% * %var% ,|)")) - continue; +static void valueFlowFunctionDefaultParameter(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) +{ + if (!tokenlist->isCPP()) + return; - // Set value in function scope.. - const unsigned int varid2 = arg->declarationId(); - for (const Token *tok2 = functionScope->classStart->next(); tok2 != functionScope->classEnd; tok2 = tok2->next()) { - if (Token::Match(tok2, "%varid% !!=", varid2)) { - for (std::list::const_iterator val = argvalues.begin(); val != argvalues.end(); ++val) - setTokenValue(const_cast(tok2), *val); - } else if (Token::Match(tok2, "%oror%|&&|{|?")) { - if (settings->debugwarnings) - bailout(tokenlist, errorLogger, tok2, "parameter " + arg->name() + ", at '" + tok2->str() + "'"); - break; - } + const std::size_t functions = symboldatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope* scope = symboldatabase->functionScopes[i]; + const Function* function = scope->function; + if (!function) + continue; + for (std::size_t arg = function->minArgCount(); arg < function->argCount(); arg++) { + const Variable* var = function->getArgumentVar(arg); + if (var && var->hasDefault() && Token::Match(var->nameToken(), "%var% = %num%|%str% [,)]")) { + const_cast(var->nameToken()->tokAt(2))->values.front().defaultArg = true; + valueFlowInjectParameter(tokenlist, errorLogger, settings, var, scope, var->nameToken()->tokAt(2)->values); } } } @@ -1704,4 +1731,5 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowSubFunction(tokenlist, errorLogger, settings); + valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings); } diff --git a/lib/valueflow.h b/lib/valueflow.h index d51354e98..744e52da0 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -30,8 +30,8 @@ class Settings; namespace ValueFlow { class Value { public: - Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false) {} - Value(const Token *c, long long val) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false) {} + Value(long long val = 0) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(0), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {} + Value(const Token *c, long long val) : intvalue(val), tokvalue(nullptr), varvalue(val), condition(c), varId(0U), conditional(false), inconclusive(false), defaultArg(false) {} /** int value */ long long intvalue; @@ -53,6 +53,9 @@ namespace ValueFlow { /** Is this value inconclusive? */ bool inconclusive; + + /** Is this value inconclusive? */ + bool defaultArg; }; void setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings); diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 8edde00c3..1e0099024 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -2306,8 +2306,8 @@ private: check("void f(int *p = 0) {\n" " if (a != 0)\n" " *p = 0;\n" - "}", false, "test.cpp", false); - ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str()); + "}", true, "test.cpp", false); + TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str()); check("void f(int *p = 0) {\n" " p = a;\n" @@ -2376,8 +2376,8 @@ private: check("void f(int *p = 0) {\n" " printf(\"%d\", p);\n" " *p = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str()); + "}", true); + ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Possible null pointer dereference if the default parameter value is used: p\n", errout.str()); // The init() function may or may not initialize p, but since the address // of p is passed in, it's a good bet that p may be modified and @@ -2414,7 +2414,7 @@ private: 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()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str()); } void nullpointer_internal_error() { // ticket #5080 diff --git a/test/testsimplifytypedef.cpp b/test/testsimplifytypedef.cpp index 70a0a2ea0..99f0fb9bc 100644 --- a/test/testsimplifytypedef.cpp +++ b/test/testsimplifytypedef.cpp @@ -1103,7 +1103,7 @@ private: "[test.cpp:20] -> [test.cpp:1]: (style, inconclusive) The function parameter 'A' hides a typedef with the same name.\n" "[test.cpp:21] -> [test.cpp:1]: (style, inconclusive) The variable 'A' hides a typedef with the same name.\n" "[test.cpp:24] -> [test.cpp:1]: (style, inconclusive) The typedef 'A' hides a typedef with the same name.\n" - "[test.cpp:24]: (debug) ValueFlow bailout: parameter a, at '{'\n", errout.str()); + "[test.cpp:21]: (debug) ValueFlow bailout: increment/decrement of a\n", errout.str()); } void simplifyTypedef36() { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index a9ed87916..fbfd968bd 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -1120,7 +1120,7 @@ private: " if (x && \n" " x->str() != y) {}\n" "}"; - TODO_ASSERT_EQUALS(true, false, testValueOfX(code, 3U, 0)); + ASSERT_EQUALS(true, testValueOfX(code, 3U, 0)); ASSERT_EQUALS(false, testValueOfX(code, 4U, 0)); // return