From c75bbbe2539f93866436af08dcb1b614adc488dd Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 8 Nov 2019 01:11:41 -0600 Subject: [PATCH] Fix issue 9404: False positive: Either the condition 'if(x)' is redundant or there is possible null pointer dereference: a->x (#2322) * Fix issue 9404: False positive: Either the condition 'if(x)' is redundant or there is possible null pointer dereference: a->x * Use simpleMatch * Add a test case for the FP * Check if expression is changed * Check for no return scope * Use simpleMatch --- lib/astutils.cpp | 44 +++++++++++++++++++++++++++++++----- lib/astutils.h | 2 +- lib/symboldatabase.cpp | 2 +- lib/templatesimplifier.cpp | 2 ++ lib/valueflow.cpp | 15 ++++++++----- test/testnullpointer.cpp | 46 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 98 insertions(+), 13 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 369759174..0427a17bf 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -952,7 +952,7 @@ static bool isEscapedOrJump(const Token* tok, bool functionsScope) return Token::Match(tok, "return|goto|throw|continue|break"); } -bool isReturnScope(const Token * const endToken, const Settings * settings, bool functionScope) +bool isReturnScope(const Token * const endToken, const Library * library, bool functionScope) { if (!endToken || endToken->str() != "}") return false; @@ -965,7 +965,7 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool if (Token::simpleMatch(prev, "}")) { if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {")) - return isReturnScope(prev, settings, functionScope) && isReturnScope(prev->link()->tokAt(-2), settings, functionScope); + return isReturnScope(prev, library, functionScope) && isReturnScope(prev->link()->tokAt(-2), library, functionScope); if (Token::simpleMatch(prev->link()->previous(), ") {") && Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") && !Token::findsimplematch(prev->link(), "break", prev)) { @@ -974,7 +974,7 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool if (isEscaped(prev->link()->astTop(), functionScope)) return true; if (Token::Match(prev->link()->previous(), "[;{}] {")) - return isReturnScope(prev, settings, functionScope); + return isReturnScope(prev, library, functionScope); } else if (Token::simpleMatch(prev, ";")) { if (Token::simpleMatch(prev->previous(), ") ;") && Token::Match(prev->linkAt(-1)->tokAt(-2), "[;{}] %name% (")) { const Token * ftok = prev->linkAt(-1)->previous(); @@ -984,8 +984,8 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool return true; if (function->isAttributeNoreturn()) return true; - } else if (settings) { - if (settings->library.isnoreturn(ftok)) + } else if (library) { + if (library->isnoreturn(ftok)) return true; } return false; @@ -1642,6 +1642,40 @@ struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const return Result(Result::Type::BAILOUT); } + if (mWhat == What::ValueFlow && Token::simpleMatch(tok, "if (") && Token::simpleMatch(tok->linkAt(1), ") {")) { + const Token *bodyStart = tok->linkAt(1)->next(); + const Token *conditionStart = tok->next(); + const Token *condTok = conditionStart->astOperand2(); + if (condTok->hasKnownIntValue()) { + bool cond = condTok->values().front().intvalue; + if (cond) { + FwdAnalysis::Result result = checkRecursive(expr, bodyStart, bodyStart->link(), exprVarIds, local, true, depth); + if (result.type != Result::Type::NONE) + return result; + } else if (Token::simpleMatch(bodyStart->link(), "} else {")) { + bodyStart = bodyStart->tokAt(2); + FwdAnalysis::Result result = checkRecursive(expr, bodyStart, bodyStart->link(), exprVarIds, local, true, depth); + if (result.type != Result::Type::NONE) + return result; + } + } + tok = bodyStart->link(); + if (isReturnScope(tok, &mLibrary)) + return Result(Result::Type::BAILOUT); + if (Token::simpleMatch(tok, "} else {")) + tok = tok->linkAt(2); + if (!tok) + return Result(Result::Type::BAILOUT); + + // Is expr changed in condition? + if (!isUnchanged(conditionStart, conditionStart->link(), exprVarIds, local)) + return Result(Result::Type::BAILOUT); + + // Is expr changed in condition body? + if (!isUnchanged(bodyStart, bodyStart->link(), exprVarIds, local)) + return Result(Result::Type::BAILOUT); + } + if (!local && Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { // TODO: this is a quick bailout return Result(Result::Type::BAILOUT); diff --git a/lib/astutils.h b/lib/astutils.h index 28e22c866..24f7b8d1a 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -126,7 +126,7 @@ bool isWithoutSideEffects(bool cpp, const Token* tok); bool isUniqueExpression(const Token* tok); /** Is scope a return scope (scope will unconditionally return) */ -bool isReturnScope(const Token *endToken, const Settings * settings = nullptr, bool functionScope=false); +bool isReturnScope(const Token * const endToken, const Library * library=nullptr, bool functionScope=false); /// Return the token to the function and the argument number const Token * getTokenArgumentFunction(const Token * tok, int& argn); diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 29cabde35..09613c068 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1401,7 +1401,7 @@ void SymbolDatabase::createSymbolDatabaseEscapeFunctions() Function * function = scope.function; if (!function) continue; - function->isEscapeFunction(isReturnScope(scope.bodyEnd, mSettings, true)); + function->isEscapeFunction(isReturnScope(scope.bodyEnd, &mSettings->library, true)); } } diff --git a/lib/templatesimplifier.cpp b/lib/templatesimplifier.cpp index 6e442039f..e7a3341d0 100644 --- a/lib/templatesimplifier.cpp +++ b/lib/templatesimplifier.cpp @@ -723,6 +723,8 @@ bool TemplateSimplifier::getTemplateDeclarations() } if (!tok1) syntaxError(tok); + if (!tok1->next()) + syntaxError(tok); // Some syntax checks, see #6865 if (!tok->tokAt(2)) syntaxError(tok->next()); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index ac287f027..437a56158 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1678,9 +1678,12 @@ static void valueFlowReverse(TokenList *tokenlist, if (Token::Match(tok2->previous(), "!!* %name% =")) { Token* assignTok = const_cast(tok2->next()->astOperand2()); if (!assignTok->hasKnownValue()) { - std::list values = {val}; setTokenValue(assignTok, val, settings); + const std::string info = "Assignment from '" + assignTok->expressionString() + "'"; + val.errorPath.emplace_back(assignTok, info); + std::list values = {val}; if (val2.condition) { + val2.errorPath.emplace_back(assignTok, info); setTokenValue(assignTok, val2, settings); values.push_back(val2); } @@ -2190,7 +2193,7 @@ static bool valueFlowForwardVariable(Token* const startToken, ++indentlevel; else if (indentlevel >= 0 && tok2->str() == "}") { --indentlevel; - if (indentlevel <= 0 && isReturnScope(tok2, settings) && Token::Match(tok2->link()->previous(), "else|) {")) { + if (indentlevel <= 0 && isReturnScope(tok2, &settings->library) && Token::Match(tok2->link()->previous(), "else|) {")) { const Token *condition = tok2->link(); const bool iselse = Token::simpleMatch(condition->tokAt(-2), "} else {"); if (iselse) @@ -2240,7 +2243,7 @@ static bool valueFlowForwardVariable(Token* const startToken, return true; } else if (indentlevel <= 0 && Token::simpleMatch(tok2->link()->previous(), "else {") && - !isReturnScope(tok2->link()->tokAt(-2), settings) && + !isReturnScope(tok2->link()->tokAt(-2), &settings->library) && isVariableChanged(tok2->link(), tok2, varid, var->isGlobal(), settings, tokenlist->isCPP())) { lowerToPossible(values); } @@ -2506,7 +2509,7 @@ static bool valueFlowForwardVariable(Token* const startToken, } // stop after conditional return scopes that are executed - if (isReturnScope(end, settings)) { + if (isReturnScope(end, &settings->library)) { std::list::iterator it; for (it = values.begin(); it != values.end();) { if (conditionIsTrue(tok2->next()->astOperand2(), getProgramMemory(tok2, varid, *it))) @@ -4375,7 +4378,7 @@ struct ValueFlowConditionHandler { continue; } - bool dead_if = isReturnScope(after, settings) || + bool dead_if = isReturnScope(after, &settings->library) || (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (") && !isBreakScope(after)); bool dead_else = false; @@ -4387,7 +4390,7 @@ struct ValueFlowConditionHandler { bailout(tokenlist, errorLogger, after, "possible noreturn scope"); continue; } - dead_else = isReturnScope(after, settings); + dead_else = isReturnScope(after, &settings->library); } if (dead_if && dead_else) diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index ed6b023e3..a3d15f308 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -82,7 +82,9 @@ private: TEST_CASE(nullpointer40); TEST_CASE(nullpointer41); TEST_CASE(nullpointer42); + TEST_CASE(nullpointer43); // #9404 TEST_CASE(nullpointer44); // #9395, #9423 + TEST_CASE(nullpointer45); TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -1538,6 +1540,17 @@ private: errout.str()); } + void nullpointer43() { + check("struct A { int* x; };\n" + "void f(A* a) {\n" + " int * x = a->x;\n" + " if (x) {\n" + " (void)*a->x;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void nullpointer44() { // #9395 check("int foo( ) {\n" @@ -1567,6 +1580,39 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointer45() { + check("struct a {\n" + " a *b() const;\n" + "};\n" + "void g() { throw 0; }\n" + "a h(a * c) {\n" + " if (c && c->b()) {}\n" + " if (!c)\n" + " g();\n" + " if (!c->b())\n" + " g();\n" + " a d = *c->b();\n" + " return d;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct a {\n" + " a *b() const;\n" + "};\n" + "void e() { throw 0; }\n" + "a f() {\n" + " a *c = 0;\n" + " if (0 && c->b()) {}\n" + " if (!c)\n" + " e();\n" + " if (!c->b())\n" + " e();\n" + " a d = *c->b();\n" + " return d;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n"