From 0f8f207719c85a733f808ff1cf07c1fcfbd05921 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 25 Jan 2021 10:24:36 -0600 Subject: [PATCH] Remove valueFlowFwdAnalysis and update valueFlowAfterAssign to handle expressions (#3074) --- lib/forwardanalyzer.cpp | 73 +++++++++++++++++++++----- lib/valueflow.cpp | 107 +++++++++++++-------------------------- test/testnullpointer.cpp | 36 +++++++++++++ 3 files changed, 131 insertions(+), 85 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index c5f6cc483..0d0e45e10 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -13,16 +13,18 @@ struct ForwardTraversal { enum class Progress { Continue, Break, Skip }; enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional }; ForwardTraversal(const ValuePtr& analyzer, const Settings* settings) - : analyzer(analyzer), settings(settings), actions(Analyzer::Action::None), analyzeOnly(false) + : analyzer(analyzer), settings(settings), actions(Analyzer::Action::None), analyzeOnly(false), analyzeTerminate(false) {} ValuePtr analyzer; const Settings* settings; Analyzer::Action actions; bool analyzeOnly; + bool analyzeTerminate; Terminate terminate = Terminate::None; - Progress Break(Terminate t = Terminate::None) { - if (!analyzeOnly && t != Terminate::None) + Progress Break(Terminate t = Terminate::None) + { + if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None) terminate = t; return Progress::Break; } @@ -224,11 +226,21 @@ struct ForwardTraversal { ft.updateRange(start, end); } - std::vector forkScope(Token* endBlock, bool isModified = false) { + ForwardTraversal forkScope(Token* endBlock, bool analyze = false) const { + ForwardTraversal ft = *this; + ft.analyzer->forkScope(endBlock); + if (analyze) { + ft.analyzeOnly = true; + ft.analyzeTerminate = true; + } + ft.updateRange(endBlock->link(), endBlock); + return ft; + } + + std::vector tryForkScope(Token* endBlock, bool isModified = false) + { if (analyzer->updateScope(endBlock, isModified)) { - ForwardTraversal ft = *this; - ft.analyzer->forkScope(endBlock); - ft.updateRange(endBlock->link(), endBlock); + ForwardTraversal ft = forkScope(endBlock); return {ft}; } return std::vector {}; @@ -238,6 +250,18 @@ struct ForwardTraversal { return Token::findsimplematch(endBlock->link(), "goto", endBlock); } + bool hasInnerReturnScope(const Token* start, const Token* end) const { + for(const Token* tok=start;tok != end;tok = tok->previous()) { + if (Token::simpleMatch(tok, "}")) { + const Token* ftok = nullptr; + bool r = isReturnScope(tok, &settings->library, &ftok); + if (r) + return true; + } + } + return false; + } + bool isEscapeScope(const Token* endBlock, bool& unknown) { const Token* ftok = nullptr; bool r = isReturnScope(endBlock, &settings->library, &ftok); @@ -259,7 +283,7 @@ struct ForwardTraversal { Analyzer::Action checkScope(Token* endBlock) { Analyzer::Action a = analyzeScope(endBlock); - forkScope(endBlock, a.isModified()); + tryForkScope(endBlock, a.isModified()); return a; } @@ -268,6 +292,31 @@ struct ForwardTraversal { return a; } + bool checkBranch(Branch& branch, Token* endBlock) { + Analyzer::Action a = analyzeScope(endBlock); + branch.action = a; + std::vector ft1 = tryForkScope(endBlock, a.isModified()); + bool bail = hasGoto(endBlock); + if (!a.isModified() && !bail) { + if (ft1.empty()) { + // Traverse into the branch to see if there is a conditional escape + if (!branch.escape && hasInnerReturnScope(endBlock->previous(), endBlock->link())) { + ForwardTraversal ft2 = forkScope(endBlock, true); + if (ft2.terminate == Terminate::Escape) { + branch.escape = true; + branch.escapeUnknown = false; + } + } + } else { + if (ft1.front().terminate == Terminate::Escape) { + branch.escape = true; + branch.escapeUnknown = false; + } + } + } + return bail; + } + void continueUpdateRangeAfterLoop(std::vector& ftv, Token* start, const Token* endToken) { for (ForwardTraversal& ft : ftv) { // If analysis has terminated normally, then continue analysis @@ -311,7 +360,7 @@ struct ForwardTraversal { return Progress::Continue; } - std::vector ftv = forkScope(endBlock, allAnalysis.isModified()); + std::vector ftv = tryForkScope(endBlock, allAnalysis.isModified()); if (bodyAnalysis.isModified()) { Token* writeTok = findRange(endBlock->link(), endBlock, std::mem_fn(&Analyzer::Action::isModified)); const Token* nextStatement = Token::findmatch(writeTok, ";|}", endBlock); @@ -451,8 +500,7 @@ struct ForwardTraversal { if (updateRange(endCond->next(), endBlock, depth - 1) == Progress::Break) return Break(); } else if (!elseBranch.check) { - thenBranch.action = checkScope(endBlock); - if (hasGoto(endBlock)) + if (checkBranch(thenBranch, endBlock)) bail = true; } // Traverse else block @@ -463,8 +511,7 @@ struct ForwardTraversal { if (result == Progress::Break) return Break(); } else if (!thenBranch.check) { - elseBranch.action = checkScope(endBlock->linkAt(2)); - if (hasGoto(endBlock)) + if (checkBranch(elseBranch, endBlock->linkAt(2))) bail = true; } tok = endBlock->linkAt(2); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b8025d93f..cd4d7b2d7 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2833,7 +2833,7 @@ static const Token* getEndOfVarScope(const Token* tok, const std::vectorisLocal()) + if (var && (var->isLocal() || var->isArgument())) endOfVarScope = var->typeStartToken()->scope()->bodyEnd; else if (!endOfVarScope) endOfVarScope = tok->scope()->bodyEnd; @@ -3812,25 +3812,22 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* } } -static void valueFlowForwardAssign(Token * const tok, - const Variable * const var, +static void valueFlowForwardAssign(Token* const tok, + const Token* expr, + std::vector vars, std::list values, - const bool constValue, - const bool init, - TokenList * const tokenlist, - ErrorLogger * const errorLogger, - const Settings * const settings) + const bool init, + TokenList* const tokenlist, + ErrorLogger* const errorLogger, + const Settings* const settings) { - const Token * endOfVarScope = nullptr; - if (var->isLocal()) - endOfVarScope = var->scope()->bodyEnd; - if (!endOfVarScope) - endOfVarScope = tok->scope()->bodyEnd; + const Token* endOfVarScope = getEndOfVarScope(tok, vars); if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) { valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); } - if (!var->isPointer() && !var->isSmartPointer()) + if (std::all_of( + vars.begin(), vars.end(), [&](const Variable* var) { return !var->isPointer() && !var->isSmartPointer(); })) values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); if (tok->astParent()) { for (ValueFlow::Value& value : values) { @@ -3839,7 +3836,7 @@ static void valueFlowForwardAssign(Token * const tok, } } - if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) { + if (tokenlist->isCPP() && vars.size() == 1 && Token::Match(vars.front()->typeStartToken(), "bool|_Bool")) { std::list::iterator it; for (it = values.begin(); it != values.end(); ++it) { if (it->isIntValue()) @@ -3850,7 +3847,7 @@ static void valueFlowForwardAssign(Token * const tok, } // Static variable initialisation? - if (var->isStatic() && init) + if (vars.size() == 1 && vars.front()->isStatic() && init) lowerToPossible(values); // Skip RHS @@ -3862,30 +3859,24 @@ static void valueFlowForwardAssign(Token * const tok, values.end(), std::back_inserter(tokvalues), std::mem_fn(&ValueFlow::Value::isTokValue)); - valueFlowForwardVariable(const_cast(nextExpression), - endOfVarScope, - var, - var->declarationId(), - tokvalues, - constValue, - false, - tokenlist, - errorLogger, - settings); + valueFlowForward(const_cast(nextExpression), endOfVarScope, expr, values, tokenlist, settings); values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); } for (ValueFlow::Value& value:values) value.tokvalue = tok; - valueFlowForwardVariable(const_cast(nextExpression), - endOfVarScope, - var, - var->declarationId(), - values, - constValue, - false, - tokenlist, - errorLogger, - settings); + valueFlowForward(const_cast(nextExpression), endOfVarScope, expr, values, tokenlist, settings); +} + +static void valueFlowForwardAssign(Token* const tok, + const Variable* const var, + const std::list& values, + const bool, + const bool init, + TokenList* const tokenlist, + ErrorLogger* const errorLogger, + const Settings* const settings) +{ + valueFlowForwardAssign(tok, var->nameToken(), {var}, values, init, tokenlist, errorLogger, settings); } static std::list truncateValues(std::list values, const ValueType *valueType, const Settings *settings) @@ -3935,7 +3926,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat for (Token* tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { // Alias if (tok->isUnaryOp("&")) { - aliased.insert(tok->astOperand1()->varId()); + aliased.insert(tok->astOperand1()->exprId()); continue; } @@ -3944,14 +3935,12 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat continue; // Lhs should be a variable - if (!tok->astOperand1() || !tok->astOperand1()->varId()) + if (!tok->astOperand1() || !tok->astOperand1()->exprId()) continue; - const int varid = tok->astOperand1()->varId(); - if (aliased.find(varid) != aliased.end()) - continue; - const Variable *var = tok->astOperand1()->variable(); - if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument())) + const int exprid = tok->astOperand1()->exprId(); + if (aliased.find(exprid) != aliased.end()) continue; + std::vector vars = getLHSVariables(tok); // Rhs values.. if (!tok->astOperand2() || tok->astOperand2()->values().empty()) @@ -3976,9 +3965,9 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat }); if (values.empty()) continue; - const bool constValue = isLiteralNumber(tok->astOperand2(), tokenlist->isCPP()); - const bool init = var->nameToken() == tok->astOperand1(); - valueFlowForwardAssign(tok->astOperand2(), var, values, constValue, init, tokenlist, errorLogger, settings); + const bool init = vars.size() == 1 && vars.front()->nameToken() == tok->astOperand1(); + valueFlowForwardAssign( + tok->astOperand2(), tok->astOperand1(), vars, values, init, tokenlist, errorLogger, settings); } } } @@ -6261,31 +6250,6 @@ struct ContainerConditionHandler : ConditionHandler { } }; -static void valueFlowFwdAnalysis(const TokenList *tokenlist, const Settings *settings) -{ - for (const Token *tok = tokenlist->front(); tok; tok = tok->next()) { - if (Token::simpleMatch(tok, "for (")) - tok = tok->linkAt(1); - if (tok->str() != "=" || !tok->astOperand1() || !tok->astOperand2()) - continue; - // Skip variables - if (tok->astOperand1()->variable()) - continue; - if (!tok->scope()->isExecutable()) - continue; - if (!tok->astOperand2()->hasKnownIntValue()) - continue; - ValueFlow::Value v(tok->astOperand2()->values().front()); - v.errorPath.emplace_back(tok, tok->astOperand1()->expressionString() + " is assigned value " + MathLib::toString(v.intvalue)); - const Token *startToken = tok->findExpressionStartEndTokens().second->next(); - const Scope *functionScope = tok->scope(); - while (functionScope->nestedIn && functionScope->nestedIn->isExecutable()) - functionScope = functionScope->nestedIn; - const Token *endToken = functionScope->bodyEnd; - valueFlowForwardExpression(const_cast(startToken), endToken, tok->astOperand1(), {v}, tokenlist, settings); - } -} - static void valueFlowDynamicBufferSize(TokenList *tokenlist, SymbolDatabase *symboldatabase, ErrorLogger *errorLogger, const Settings *settings) { for (const Scope *functionScope : symboldatabase->functionScopes) { @@ -6663,7 +6627,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowBitAnd(tokenlist); valueFlowSameExpressions(tokenlist); valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings); - valueFlowFwdAnalysis(tokenlist, settings); std::size_t values = 0; std::size_t n = 4; diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 22e39a360..7b4ae76c8 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -110,6 +110,7 @@ private: TEST_CASE(nullpointer67); // #10062 TEST_CASE(nullpointer68); TEST_CASE(nullpointer69); // #8143 + TEST_CASE(nullpointer70); TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -2177,6 +2178,41 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointer70() { + check("struct Token {\n" + " const Token* nextArgument() const;\n" + " const Token* next() const;\n" + " int varId() const;\n" + "};\n" + "int f(const Token *first, const Token* second) const {\n" + " first = first->nextArgument();\n" + " if (first)\n" + " first = first->next();\n" + " if (second->next()->varId() == 0) {\n" + " second = second->nextArgument();\n" + " if (!first || !second)\n" + " return 0;\n" + " } else if (!first) {\n" + " return 0;\n" + " }\n" + " return first->varId();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct Token {\n" + " const Token* nextArgument() const;\n" + " const Token* next() const;\n" + " int varId() const;\n" + "};\n" + "int f(const Token *first) const {\n" + " first = first->nextArgument();\n" + " if (first)\n" + " first = first->next();\n" + " first->str();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (warning) Either the condition 'first' is redundant or there is possible null pointer dereference: first.\n", errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n"