From dd178c3ad9d060d2daa29d3683f9f1cf55ca7145 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 19 Jun 2021 06:59:48 -0500 Subject: [PATCH] Fix 10314: Possible nullPointerRedundantCheck false positive (#3298) --- lib/analyzer.h | 10 +++ lib/astutils.cpp | 3 +- lib/forwardanalyzer.cpp | 74 +++++++++--------- lib/forwardanalyzer.h | 10 +-- lib/valueflow.cpp | 169 +++++++++++++++++++++------------------- test/testvalueflow.cpp | 23 ++++++ 6 files changed, 167 insertions(+), 122 deletions(-) diff --git a/lib/analyzer.h b/lib/analyzer.h index 33fc71883..8d0f5be7d 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -111,6 +111,16 @@ struct Analyzer { unsigned int mFlag; }; + enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional }; + + struct Result { + Result(Action action = Action::None, Terminate terminate = Terminate::None) + : action(action), terminate(terminate) + {} + Action action; + Terminate terminate; + }; + enum class Direction { Forward, Reverse }; struct Assume { diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 8fbc2d618..a220cfb99 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1563,7 +1563,8 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {")) return isReturnScope(prev, library, unknownFunc, functionScope) && isReturnScope(prev->link()->tokAt(-2), library, unknownFunc, functionScope); - if (Token::simpleMatch(prev->link()->previous(), ") {") && + // TODO: Check all cases + if (!functionScope && Token::simpleMatch(prev->link()->previous(), ") {") && Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") && !Token::findsimplematch(prev->link(), "break", prev)) { return true; diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 177eb33ab..617019ee1 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -22,11 +22,12 @@ struct ForwardTraversal { Analyzer::Action actions; bool analyzeOnly; bool analyzeTerminate; - Terminate terminate = Terminate::None; + Analyzer::Terminate terminate = Analyzer::Terminate::None; bool forked = false; - Progress Break(Terminate t = Terminate::None) { - if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None) + Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) + { + if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None) terminate = t; return Progress::Break; } @@ -89,7 +90,7 @@ struct ForwardTraversal { else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) { traverseRecursive(tok->astOperand1(), f, traverseUnknown); traverseRecursive(tok->astOperand2(), f, traverseUnknown); - return Break(Terminate::Escape); + return Break(Analyzer::Terminate::Escape); } else if (isUnevaluated(tok)) { if (out) *out = tok->link(); @@ -103,7 +104,7 @@ struct ForwardTraversal { // Skip lambdas } else if (T* lambdaEndToken = findLambdaEndToken(tok)) { if (checkScope(lambdaEndToken).isModified()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); if (out) *out = lambdaEndToken->next(); // Skip class scope @@ -152,7 +153,7 @@ struct ForwardTraversal { if (!checkThen && !checkElse) { // Stop if the value is conditional if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) { - return Break(Terminate::Conditional); + return Break(Analyzer::Terminate::Conditional); } checkThen = true; checkElse = true; @@ -180,12 +181,12 @@ struct ForwardTraversal { if (!action.isNone() && !analyzeOnly) analyzer->update(tok, action, Analyzer::Direction::Forward); if (action.isInconclusive() && !analyzer->lowerToInconclusive()) - return Break(Terminate::Inconclusive); + return Break(Analyzer::Terminate::Inconclusive); if (action.isInvalid()) - return Break(Terminate::Modified); + return Break(Analyzer::Terminate::Modified); if (action.isWrite() && !action.isRead()) // Analysis of this write will continue separately - return Break(Terminate::Modified); + return Break(Analyzer::Terminate::Modified); return Progress::Continue; } @@ -320,13 +321,13 @@ struct ForwardTraversal { if (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) { ForwardTraversal ft2 = fork(true); ft2.updateScope(branch.endBlock); - if (ft2.terminate == Terminate::Escape) { + if (ft2.terminate == Analyzer::Terminate::Escape) { branch.escape = true; branch.escapeUnknown = false; } } } else { - if (ft1.front().terminate == Terminate::Escape) { + if (ft1.front().terminate == Analyzer::Terminate::Escape) { branch.escape = true; branch.escapeUnknown = false; } @@ -387,10 +388,10 @@ struct ForwardTraversal { actions |= allAnalysis; if (allAnalysis.isInconclusive()) { if (!analyzer->lowerToInconclusive()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (allAnalysis.isModified()) { if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } bool checkThen = true; bool checkElse = false; @@ -409,9 +410,9 @@ struct ForwardTraversal { return Break(); // If loop re-enters then it could be modified again if (allAnalysis.isModified() && reentersLoop(endBlock, condTok, stepTok)) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); if (allAnalysis.isIncremental()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else { std::vector ftv = tryForkScope(endBlock, allAnalysis.isModified()); bool forkContinue = true; @@ -425,9 +426,9 @@ struct ForwardTraversal { if (allAnalysis.isModified() || !forkContinue) { // TODO: Dont bail on missing condition if (!condTok) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); if (analyzer->isConditional() && stopUpdates()) - return Break(Terminate::Conditional); + return Break(Analyzer::Terminate::Conditional); analyzer->assume(condTok, false); } if (forkContinue) { @@ -437,7 +438,7 @@ struct ForwardTraversal { } } if (allAnalysis.isIncremental()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } return Progress::Continue; } @@ -451,7 +452,7 @@ struct ForwardTraversal { Progress updateRange(Token* start, const Token* end, int depth = 20) { forked = false; if (depth < 0) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); std::size_t i = 0; for (Token* tok = start; tok && tok != end; tok = tok->next()) { Token* next = nullptr; @@ -485,13 +486,13 @@ struct ForwardTraversal { return Break(); tok = skipTo(tok, scopeEndToken, end); if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); // TODO: Don't break, instead move to the outer scope if (!tok) return Break(); } else if (Token::Match(tok, "%name% :") || tok->str() == "case") { if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (tok->link() && tok->str() == "}") { const Scope* scope = tok->scope(); if (!scope) @@ -505,7 +506,7 @@ struct ForwardTraversal { return Break(); if (!condTok->hasKnownIntValue() || inLoop) { if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (condTok->values().front().intvalue == inElse) { return Break(); } @@ -524,7 +525,7 @@ struct ForwardTraversal { tok = tok->linkAt(2); } else if (scope->type == Scope::eTry) { if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (scope->type == Scope::eLambda) { return Break(); } else if (scope->type == Scope::eDo && Token::simpleMatch(tok, "} while (")) { @@ -595,32 +596,32 @@ struct ForwardTraversal { return Break(); if (thenBranch.isDead() && elseBranch.isDead()) { if (thenBranch.isModified() && elseBranch.isModified()) - return Break(Terminate::Modified); + return Break(Analyzer::Terminate::Modified); if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape()) - return Break(Terminate::Escape); - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Escape); + return Break(Analyzer::Terminate::Bail); } // Conditional return if (thenBranch.isEscape() && !hasElse) { if (!thenBranch.isConclusiveEscape()) { if (!analyzer->lowerToInconclusive()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (thenBranch.check) { return Break(); } else { if (analyzer->isConditional() && stopUpdates()) - return Break(Terminate::Conditional); + return Break(Analyzer::Terminate::Conditional); analyzer->assume(condTok, false); } } if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) { if (!analyzer->lowerToInconclusive()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); } else if (thenBranch.isModified() || elseBranch.isModified()) { if (!hasElse && analyzer->isConditional() && stopUpdates()) - return Break(Terminate::Conditional); + return Break(Analyzer::Terminate::Conditional); if (!analyzer->lowerToPossible()) - return Break(Terminate::Bail); + return Break(Analyzer::Terminate::Bail); analyzer->assume(condTok, elseBranch.isModified()); } } @@ -742,19 +743,16 @@ struct ForwardTraversal { }; -Analyzer::Action valueFlowGenericForward(Token* start, - const Token* end, - const ValuePtr& a, - const Settings* settings) +Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr& a, const Settings* settings) { ForwardTraversal ft{a, settings}; ft.updateRange(start, end); - return ft.actions; + return {ft.actions, ft.terminate}; } -Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr& a, const Settings* settings) +Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr& a, const Settings* settings) { ForwardTraversal ft{a, settings}; ft.updateRecursive(start); - return ft.actions; + return {ft.actions, ft.terminate}; } diff --git a/lib/forwardanalyzer.h b/lib/forwardanalyzer.h index 3238cfa0c..c3aa58669 100644 --- a/lib/forwardanalyzer.h +++ b/lib/forwardanalyzer.h @@ -25,11 +25,11 @@ class Settings; class Token; template class ValuePtr; -Analyzer::Action valueFlowGenericForward(Token* start, - const Token* end, - const ValuePtr& a, - const Settings* settings); +Analyzer::Result valueFlowGenericForward(Token* start, + const Token* end, + const ValuePtr& a, + const Settings* settings); -Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr& a, const Settings* settings); +Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr& a, const Settings* settings); #endif diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 2b1f1d831..33e584e4b 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1664,12 +1664,12 @@ static void valueFlowGlobalStaticVar(TokenList *tokenList, const Settings *setti } } -static Analyzer::Action valueFlowForwardVariable(Token* const startToken, - const Token* const endToken, - const Variable* const var, - std::list values, - TokenList* const tokenlist, - const Settings* const settings); +static Analyzer::Result valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + TokenList* const tokenlist, + const Settings* const settings); static void valueFlowReverse(TokenList* tokenlist, Token* tok, @@ -2407,28 +2407,32 @@ static std::vector getAliasesFromValues(std::list values, - std::vector aliases, - TokenList* const tokenlist, - const Settings* const settings) +static Analyzer::Result valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + std::vector aliases, + TokenList* const tokenlist, + const Settings* const settings) { Analyzer::Action actions; + Analyzer::Terminate terminate = Analyzer::Terminate::None; for (ValueFlow::Value& v : values) { VariableAnalyzer a(var, v, aliases, tokenlist); - actions |= valueFlowGenericForward(startToken, endToken, a, settings); + Analyzer::Result r = valueFlowGenericForward(startToken, endToken, a, settings); + actions |= r.action; + if (terminate == Analyzer::Terminate::None) + terminate = r.terminate; } - return actions; + return {actions, terminate}; } -static Analyzer::Action valueFlowForwardVariable(Token* const startToken, - const Token* const endToken, - const Variable* const var, - std::list values, - TokenList* const tokenlist, - const Settings* const settings) +static Analyzer::Result valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + TokenList* const tokenlist, + const Settings* const settings) { auto aliases = getAliasesFromValues(values); return valueFlowForwardVariable( @@ -2527,19 +2531,23 @@ struct OppositeExpressionAnalyzer : ExpressionAnalyzer { } }; -static Analyzer::Action valueFlowForwardExpression(Token* startToken, - const Token* endToken, - const Token* exprTok, - const std::list& values, - const TokenList* const tokenlist, - const Settings* settings) +static Analyzer::Result valueFlowForwardExpression(Token* startToken, + const Token* endToken, + const Token* exprTok, + const std::list& values, + const TokenList* const tokenlist, + const Settings* settings) { Analyzer::Action actions; + Analyzer::Terminate terminate = Analyzer::Terminate::None; for (const ValueFlow::Value& v : values) { ExpressionAnalyzer a(exprTok, v, tokenlist); - actions |= valueFlowGenericForward(startToken, endToken, a, settings); + Analyzer::Result r = valueFlowGenericForward(startToken, endToken, a, settings); + actions |= r.action; + if (terminate == Analyzer::Terminate::None) + terminate = r.terminate; } - return actions; + return {actions, terminate}; } static const Token* parseBinaryIntOp(const Token* expr, MathLib::bigint& known) @@ -2614,12 +2622,12 @@ ValuePtr makeAnalyzer(Token* exprTok, const ValueFlow::Value& value, c } } -static Analyzer::Action valueFlowForward(Token* startToken, - const Token* endToken, - const Token* exprTok, - std::list values, - TokenList* const tokenlist, - const Settings* settings) +static Analyzer::Result valueFlowForward(Token* startToken, + const Token* endToken, + const Token* exprTok, + std::list values, + TokenList* const tokenlist, + const Settings* settings) { const Token* expr = solveExprValues(exprTok, values); if (expr->variable()) { @@ -4285,12 +4293,12 @@ struct ConditionHandler { Condition() : vartok(nullptr), true_values(), false_values(), inverted(false) {} }; - virtual bool forward(Token* start, - const Token* stop, - const Token* exprTok, - const std::list& values, - TokenList* tokenlist, - const Settings* settings) const = 0; + virtual Analyzer::Result forward(Token* start, + const Token* stop, + const Token* exprTok, + const std::list& values, + TokenList* tokenlist, + const Settings* settings) const = 0; virtual void reverse(Token* start, const Token* endToken, @@ -4577,9 +4585,6 @@ struct ConditionHandler { return; } - // start token of conditional code - Token* startTokens[] = {nullptr, nullptr}; - // if astParent is "!" we need to invert codeblock { const Token* tok2 = tok; @@ -4593,6 +4598,9 @@ struct ConditionHandler { } } + bool deadBranch[] = {false, false}; + // start token of conditional code + Token* startTokens[] = {nullptr, nullptr}; // determine startToken(s) if (Token::simpleMatch(top->link(), ") {")) startTokens[0] = top->link()->next(); @@ -4608,12 +4616,13 @@ struct ConditionHandler { std::list& values = (i == 0 ? thenValues : elseValues); valueFlowSetConditionToKnown(tok, values, i == 0); - // TODO: The endToken should not be startTokens[i]->link() in the valueFlowForwardVariable call - if (forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings)) + Analyzer::Result r = + forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings); + deadBranch[i] = r.terminate == Analyzer::Terminate::Escape; + if (r.action.isModified() && !deadBranch[i]) changeBlock = i; changeKnownToPossible(values); } - // TODO: Values changed in noreturn blocks should not bail if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) { if (settings->debugwarnings) bailout(tokenlist, @@ -4627,12 +4636,13 @@ struct ConditionHandler { // After conditional code.. if (Token::simpleMatch(top->link(), ") {")) { Token* after = top->link()->linkAt(1); + bool dead_if = deadBranch[0]; + bool dead_else = deadBranch[1]; const Token* unknownFunction = nullptr; - const bool isWhile = - tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while ("); - bool dead_if = (!isBreakScope(after) && isWhile) || - (isReturnScope(after, &settings->library, &unknownFunction) && !isWhile); - bool dead_else = false; + if (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (")) + dead_if = !isBreakScope(after); + else if (!dead_if) + dead_if = isReturnScope(after, &settings->library, &unknownFunction); if (!dead_if && unknownFunction) { if (settings->debugwarnings) @@ -4643,7 +4653,8 @@ struct ConditionHandler { if (Token::simpleMatch(after, "} else {")) { after = after->linkAt(2); unknownFunction = nullptr; - dead_else = isReturnScope(after, &settings->library, &unknownFunction); + if (!dead_else) + dead_else = isReturnScope(after, &settings->library, &unknownFunction); if (!dead_else && unknownFunction) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); @@ -4717,13 +4728,14 @@ static void valueFlowCondition(const ValuePtr& handler, } struct SimpleConditionHandler : ConditionHandler { - virtual bool forward(Token* start, - const Token* stop, - const Token* exprTok, - const std::list& values, - TokenList* tokenlist, - const Settings* settings) const OVERRIDE { - return valueFlowForward(start->next(), stop, exprTok, values, tokenlist, settings).isModified(); + virtual Analyzer::Result forward(Token* start, + const Token* stop, + const Token* exprTok, + const std::list& values, + TokenList* tokenlist, + const Settings* settings) const OVERRIDE + { + return valueFlowForward(start->next(), stop, exprTok, values, tokenlist, settings); } virtual void reverse(Token* start, @@ -6081,19 +6093,19 @@ struct ContainerVariableAnalyzer : VariableAnalyzer { } }; -static Analyzer::Action valueFlowContainerForward(Token* tok, - const Token* endToken, - const Variable* var, - ValueFlow::Value value, - TokenList* tokenlist) +static Analyzer::Result valueFlowContainerForward(Token* tok, + const Token* endToken, + const Variable* var, + ValueFlow::Value value, + TokenList* tokenlist) { ContainerVariableAnalyzer a(var, value, getAliasesFromValues({value}), tokenlist); return valueFlowGenericForward(tok, endToken, a, tokenlist->getSettings()); } -static Analyzer::Action valueFlowContainerForward(Token* tok, - const Variable* var, - ValueFlow::Value value, - TokenList* tokenlist) +static Analyzer::Result valueFlowContainerForward(Token* tok, + const Variable* var, + ValueFlow::Value value, + TokenList* tokenlist) { const Token * endOfVarScope = nullptr; if (var->isLocal() || var->isArgument()) @@ -6506,19 +6518,20 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold } struct ContainerConditionHandler : ConditionHandler { - virtual bool forward(Token* start, - const Token* stop, - const Token* exprTok, - const std::list& values, - TokenList* tokenlist, - const Settings*) const OVERRIDE { + virtual Analyzer::Result forward(Token* start, + const Token* stop, + const Token* exprTok, + const std::list& values, + TokenList* tokenlist, + const Settings*) const OVERRIDE + { // TODO: Forward multiple values if (values.empty()) - return false; + return {}; const Variable* var = exprTok->variable(); if (!var) - return false; - return valueFlowContainerForward(start->next(), stop, var, values.front(), tokenlist).isModified(); + return {}; + return valueFlowContainerForward(start->next(), stop, var, values.front(), tokenlist); } virtual void reverse(Token* start, diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 00b5dcba1..52b351282 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -2718,6 +2718,29 @@ private: " return x;\n" "}\n"; ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0)); + + code = "int g(int x) {\n" + " switch (x) {\n" + " case 1:\n" + " return 1;\n" + " default:\n" + " return 2;\n" + " }\n" + "}\n" + "void f(int x) {\n" + " if (x == 3)\n" + " x = g(0);\n" + " int a = x;\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 12U, 3)); + + code = "int g(int x) { throw 0; }\n" + "void f(int x) {\n" + " if (x == 3)\n" + " x = g(0);\n" + " int a = x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXImpossible(code, 5U, 3)); } void valueFlowAfterConditionExpr() {