From 25ada657daad6953e6deaa0e27fb2d5b458b4935 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 18 Jan 2021 03:12:07 -0600 Subject: [PATCH] Fix issue 9030: ValueFlow: Possible value after conditional assignment in for loop (#3059) --- lib/forwardanalyzer.cpp | 188 ++++++++++++++++++++++++---------------- lib/programmemory.cpp | 4 +- lib/valueflow.cpp | 30 ++++++- test/testuninitvar.cpp | 26 ++++++ test/testvalueflow.cpp | 17 +++- 5 files changed, 182 insertions(+), 83 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 8d52d2402..75c1692a2 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -11,6 +11,7 @@ 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) {} @@ -18,6 +19,14 @@ struct ForwardTraversal { const Settings* settings; Analyzer::Action actions; bool analyzeOnly; + Terminate terminate = Terminate::None; + + Progress Break(Terminate t = Terminate::None) + { + if (!analyzeOnly && t != Terminate::None) + terminate = t; + return Progress::Break; + } struct Branch { Analyzer::Action action = Analyzer::Action::None; @@ -62,25 +71,25 @@ struct ForwardTraversal { template)> Progress traverseTok(T* tok, std::function f, bool traverseUnknown, T** out = nullptr) { if (Token::Match(tok, "asm|goto|continue|setjmp|longjmp")) - return Progress::Break; + return Break(); else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) { traverseRecursive(tok->astOperand1(), f, traverseUnknown); traverseRecursive(tok->astOperand2(), f, traverseUnknown); - return Progress::Break; + return Break(Terminate::Escape); } else if (isUnevaluated(tok)) { if (out) *out = tok->link(); return Progress::Skip; } else if (tok->astOperand1() && tok->astOperand2() && Token::Match(tok, "?|&&|%oror%")) { if (traverseConditional(tok, f, traverseUnknown) == Progress::Break) - return Progress::Break; + return Break(); if (out) *out = nextAfterAstRightmostLeaf(tok); return Progress::Skip; // Skip lambdas } else if (T* lambdaEndToken = findLambdaEndToken(tok)) { if (checkScope(lambdaEndToken).isModified()) - return Progress::Break; + return Break(Terminate::Bail); if (out) *out = lambdaEndToken->next(); // Skip class scope @@ -89,7 +98,7 @@ struct ForwardTraversal { *out = tok->link(); } else { if (f(tok) == Progress::Break) - return Progress::Break; + return Break(); } return Progress::Continue; } @@ -106,14 +115,14 @@ struct ForwardTraversal { if (tok->isAssignmentOp()) std::swap(firstOp, secondOp); if (firstOp && traverseRecursive(firstOp, f, traverseUnknown, recursion+1) == Progress::Break) - return Progress::Break; + return Break(); Progress p = tok->isAssignmentOp() ? Progress::Continue : traverseTok(tok, f, traverseUnknown); if (p == Progress::Break) - return Progress::Break; + return Break(); if (p == Progress::Continue && secondOp && traverseRecursive(secondOp, f, traverseUnknown, recursion+1) == Progress::Break) - return Progress::Break; + return Break(); if (tok->isAssignmentOp() && traverseTok(tok, f, traverseUnknown) == Progress::Break) - return Progress::Break; + return Break(); return Progress::Continue; } @@ -126,23 +135,24 @@ struct ForwardTraversal { std::tie(checkThen, checkElse) = evalCond(condTok); if (!checkThen && !checkElse) { // Stop if the value is conditional - if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) - return Progress::Break; + if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) { + return Break(Terminate::Conditional); + } checkThen = true; checkElse = true; } if (childTok->str() == ":") { if (checkThen && traverseRecursive(childTok->astOperand1(), f, traverseUnknown) == Progress::Break) - return Progress::Break; + return Break(); if (checkElse && traverseRecursive(childTok->astOperand2(), f, traverseUnknown) == Progress::Break) - return Progress::Break; + return Break(); } else { if (!checkThen && tok->str() == "&&") return Progress::Continue; if (!checkElse && tok->str() == "||") return Progress::Continue; if (traverseRecursive(childTok, f, traverseUnknown) == Progress::Break) - return Progress::Break; + return Break(); } } return Progress::Continue; @@ -154,12 +164,12 @@ struct ForwardTraversal { if (!action.isNone() && !analyzeOnly) analyzer->update(tok, action, Analyzer::Direction::Forward); if (action.isInconclusive() && !analyzer->lowerToInconclusive()) - return Progress::Break; + return Break(Terminate::Inconclusive); if (action.isInvalid()) - return Progress::Break; + return Break(Terminate::Modified); if (action.isWrite() && !action.isRead()) // Analysis of this write will continue separately - return Progress::Break; + return Break(Terminate::Modified); return Progress::Continue; } @@ -192,7 +202,7 @@ struct ForwardTraversal { std::function f = [&](const Token* tok) { result = analyzer->analyze(tok, Analyzer::Direction::Forward); if (result.isModified() || result.isInconclusive()) - return Progress::Break; + return Break(); return Progress::Continue; }; traverseRecursive(start, f, true); @@ -205,7 +215,7 @@ struct ForwardTraversal { Analyzer::Action action = analyzer->analyze(tok, Analyzer::Direction::Forward); if (action.isModified() || action.isInconclusive()) return action; - result = action; + result |= action; } return result; } @@ -215,12 +225,15 @@ struct ForwardTraversal { ft.updateRange(start, end); } - void forkScope(Token* endBlock, bool isModified = false) { + std::vector forkScope(Token* endBlock, bool isModified = false) + { if (analyzer->updateScope(endBlock, isModified)) { ForwardTraversal ft = *this; ft.analyzer->forkScope(endBlock); ft.updateRange(endBlock->link(), endBlock); + return {ft}; } + return std::vector{}; } static bool hasGoto(const Token* endBlock) { @@ -257,7 +270,21 @@ struct ForwardTraversal { return a; } - Progress updateLoop(Token* endBlock, Token* condTok, Token* initTok = nullptr, Token* stepTok = nullptr) { + void continueUpdateRangeAfterLoop(std::vector& ftv, Token* start, const Token* endToken) + { + for (ForwardTraversal& ft : ftv) { + // If analysis has terminated normally, then continue analysis + if (ft.terminate == Terminate::None) + ft.updateRange(start, endToken); + } + } + + Progress updateLoop(const Token* endToken, + Token* endBlock, + Token* condTok, + Token* initTok = nullptr, + Token* stepTok = nullptr) + { const bool isDoWhile = precedes(endBlock, condTok); Analyzer::Action bodyAnalysis = analyzeScope(endBlock); Analyzer::Action allAnalysis = bodyAnalysis; @@ -270,32 +297,37 @@ struct ForwardTraversal { actions |= allAnalysis; if (allAnalysis.isInconclusive()) { if (!analyzer->lowerToInconclusive()) - return Progress::Break; + return Break(Terminate::Bail); } else if (allAnalysis.isModified()) { if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); } // Traverse condition after lowering - if (condTok && (!isDoWhile || !bodyAnalysis.isModified())) { + if (condTok && (!isDoWhile || (!bodyAnalysis.isModified() && !bodyAnalysis.isIdempotent()))) { if (updateRecursive(condTok) == Progress::Break) - return Progress::Break; + return Break(); - bool checkThen, checkElse; + bool checkThen = true; + bool checkElse = false; std::tie(checkThen, checkElse) = evalCond(condTok); + // condition is false, we don't enter the loop if (checkElse) - // condition is false, we don't enter the loop - return Progress::Break; + return Progress::Continue; } - forkScope(endBlock, allAnalysis.isModified()); + std::vector ftv = forkScope(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); - if (!Token::Match(nextStatement, ";|} break ;")) - return Progress::Break; + if (!Token::Match(nextStatement, ";|} break ;")) { + continueUpdateRangeAfterLoop(ftv, endBlock, endToken); + return Break(Terminate::Bail); + } } else { - if (stepTok && updateRecursive(stepTok) == Progress::Break) - return Progress::Break; + if (stepTok && updateRecursive(stepTok) == Progress::Break) { + continueUpdateRangeAfterLoop(ftv, endBlock, endToken); + return Break(Terminate::Bail); + } } // TODO: Should we traverse the body? // updateRange(endBlock->link(), endBlock); @@ -322,39 +354,39 @@ struct ForwardTraversal { // Evaluate RHS of assignment before LHS if (Token* assignTok = assignExpr(tok)) { if (updateRecursive(assignTok) == Progress::Break) - return Progress::Break; + return Break(); tok = nextAfterAstRightmostLeaf(assignTok); if (!tok) - return Progress::Break; + return Break(); } else if (tok->str() == "break") { const Token *scopeEndToken = findNextTokenFromBreak(tok); if (!scopeEndToken) - return Progress::Break; + return Break(); tok = skipTo(tok, scopeEndToken, end); if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); // TODO: Don't break, instead move to the outer scope if (!tok) - return Progress::Break; + return Break(); } else if (Token::Match(tok, "%name% :") || tok->str() == "case") { if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); } else if (tok->link() && tok->str() == "}") { const Scope* scope = tok->scope(); if (!scope) - return Progress::Break; + return Break(); if (Token::Match(tok->link()->previous(), ")|else {")) { const Token* tok2 = tok->link()->previous(); const bool inElse = Token::simpleMatch(tok2, "else {"); const bool inLoop = inElse ? false : Token::Match(tok2->link()->previous(), "while|for ("); Token* condTok = getCondTokFromEnd(tok); if (!condTok) - return Progress::Break; + return Break(); if (!condTok->hasKnownIntValue() || inLoop) { if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); } else if (condTok->values().front().intvalue == inElse) { - return Progress::Break; + return Break(); } // Handle for loop Token* stepTok = getStepTokFromEnd(tok); @@ -362,21 +394,21 @@ struct ForwardTraversal { std::tie(checkThen, checkElse) = evalCond(condTok); if (stepTok && !checkElse) { if (updateRecursive(stepTok) == Progress::Break) - return Progress::Break; + return Break(); if (updateRecursive(condTok) == Progress::Break) - return Progress::Break; + return Break(); } analyzer->assume(condTok, !inElse, tok); if (Token::simpleMatch(tok, "} else {")) tok = tok->linkAt(2); } else if (scope->type == Scope::eTry) { if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); } else if (scope->type == Scope::eLambda) { - return Progress::Break; + return Break(); } else if (scope->type == Scope::eDo && Token::simpleMatch(tok, "} while (")) { - if (updateLoop(tok, tok->tokAt(2)->astOperand2()) == Progress::Break) - return Progress::Break; + if (updateLoop(end, tok, tok->tokAt(2)->astOperand2()) == Progress::Break) + return Break(); tok = tok->linkAt(2); } else if (Token::simpleMatch(tok->next(), "else {")) { tok = tok->linkAt(2); @@ -387,26 +419,25 @@ struct ForwardTraversal { Token* condTok = getCondTok(tok); Token* initTok = getInitTok(tok); if (initTok && updateRecursive(initTok) == Progress::Break) - return Progress::Break; + return Break(); if (Token::Match(tok, "for|while (")) { // For-range loop if (Token::simpleMatch(condTok, ":")) { Token* conTok = condTok->astOperand2(); if (conTok && updateRecursive(conTok) == Progress::Break) - return Progress::Break; - if (updateLoop(endBlock, condTok) == Progress::Break) - return Progress::Break; + return Break(); + if (updateLoop(end, endBlock, condTok) == Progress::Break) + return Break(); } else { Token* stepTok = getStepTok(tok); - if (updateLoop(endBlock, condTok, initTok, stepTok) == Progress::Break) - return Progress::Break; - + if (updateLoop(end, endBlock, condTok, initTok, stepTok) == Progress::Break) + return Break(); } tok = endBlock; } else { // Traverse condition if (updateRecursive(condTok) == Progress::Break) - return Progress::Break; + return Break(); Branch thenBranch{}; Branch elseBranch{}; // Check if condition is true or false @@ -418,7 +449,7 @@ struct ForwardTraversal { thenBranch.escape = isEscapeScope(endBlock, thenBranch.escapeUnknown); if (thenBranch.check) { if (updateRange(endCond->next(), endBlock) == Progress::Break) - return Progress::Break; + return Break(); } else if (!elseBranch.check) { thenBranch.action = checkScope(endBlock); if (hasGoto(endBlock)) @@ -430,7 +461,7 @@ struct ForwardTraversal { if (elseBranch.check) { Progress result = updateRange(endBlock->tokAt(2), endBlock->linkAt(2)); if (result == Progress::Break) - return Progress::Break; + return Break(); } else if (!thenBranch.check) { elseBranch.action = checkScope(endBlock->linkAt(2)); if (hasGoto(endBlock)) @@ -442,30 +473,35 @@ struct ForwardTraversal { } actions |= (thenBranch.action | elseBranch.action); if (bail) - return Progress::Break; - if (thenBranch.isDead() && elseBranch.isDead()) - return Progress::Break; + return Break(); + if (thenBranch.isDead() && elseBranch.isDead()) { + if (thenBranch.isModified() && elseBranch.isModified()) + return Break(Terminate::Modified); + if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape()) + return Break(Terminate::Escape); + return Break(Terminate::Bail); + } // Conditional return if (thenBranch.isEscape() && !hasElse) { if (!thenBranch.isConclusiveEscape()) { if (!analyzer->lowerToInconclusive()) - return Progress::Break; + return Break(Terminate::Bail); } else if (thenBranch.check) { - return Progress::Break; + return Break(); } else { if (analyzer->isConditional() && stopUpdates()) - return Progress::Break; + return Break(Terminate::Conditional); analyzer->assume(condTok, false); } } if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) { if (!analyzer->lowerToInconclusive()) - return Progress::Break; + return Break(Terminate::Bail); } else if (thenBranch.isModified() || elseBranch.isModified()) { if (!hasElse && analyzer->isConditional() && stopUpdates()) - return Progress::Break; + return Break(Terminate::Conditional); if (!analyzer->lowerToPossible()) - return Progress::Break; + return Break(Terminate::Bail); analyzer->assume(condTok, elseBranch.isModified()); } } @@ -473,15 +509,15 @@ struct ForwardTraversal { Token* endBlock = tok->next()->link(); Analyzer::Action a = analyzeScope(endBlock); if (updateRange(tok->next(), endBlock) == Progress::Break) - return Progress::Break; + return Break(); if (a.isModified()) analyzer->lowerToPossible(); tok = endBlock; } else if (Token::simpleMatch(tok, "do {")) { Token* endBlock = tok->next()->link(); Token* condTok = Token::simpleMatch(endBlock, "} while (") ? endBlock->tokAt(2)->astOperand2() : nullptr; - if (updateLoop(endBlock, condTok) == Progress::Break) - return Progress::Break; + if (updateLoop(end, endBlock, condTok) == Progress::Break) + return Break(); if (condTok) tok = endBlock->linkAt(2)->next(); else @@ -491,21 +527,21 @@ struct ForwardTraversal { bool checkThen, checkElse; std::tie(checkThen, checkElse) = evalCond(condTok); if (checkElse) - return Progress::Break; + return Break(); if (!checkThen) analyzer->assume(condTok, true, tok); } else if (Token::simpleMatch(tok, "switch (")) { if (updateRecursive(tok->next()->astOperand2()) == Progress::Break) - return Progress::Break; - return Progress::Break; + return Break(); + return Break(); } else { if (updateTok(tok, &next) == Progress::Break) - return Progress::Break; + return Break(); if (next) { if (precedes(next, end)) tok = next->previous(); else - return Progress::Break; + return Break(); } } // Prevent infinite recursion diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 2f0451fa9..e15428e74 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -209,8 +209,8 @@ static void fillProgramMemoryFromAssignments(ProgramMemory& pm, const Token* tok setvar = true; } } - if (!setvar && (Token::Match(tok2, ";|{|}|%type% %var% =") || - Token::Match(tok2, "[;{}] const| %type% %var% ("))) { + if (!setvar && (Token::Match(tok2, ";|{|}|%type% %var% =") || Token::Match(tok2, "[;{}] const| %type% %var% (") || + Token::Match(tok2->previous(), "for ( %var% ="))) { const Token *vartok = tok2->next(); while (vartok->next()->isName()) vartok = vartok->next(); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index f41db5731..2c195456b 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1730,7 +1730,30 @@ static const ValueFlow::Value* getKnownValue(const Token* tok, ValueFlow::Value: return nullptr; } -static bool bifurcate(const Token* tok, const std::set& varids, const Settings* settings, int depth = 20) +static bool bifurcate(const Token* tok, const std::set& varids, const Settings* settings, int depth = 20); + +static bool bifurcateVariableChanged(const Variable* var, + const std::set& varids, + const Token* start, + const Token* end, + const Settings* settings, + int depth = 20) +{ + bool result = false; + const Token* tok = start; + while ((tok = findVariableChanged( + tok->next(), end, var->isPointer(), var->declarationId(), var->isGlobal(), settings, true))) { + if (Token::Match(tok->astParent(), "%assign%")) { + if (!bifurcate(tok->astParent()->astOperand2(), varids, settings, depth - 1)) + return true; + } else { + result = true; + } + } + return result; +} + +static bool bifurcate(const Token* tok, const std::set& varids, const Settings* settings, int depth) { if (depth < 0) return false; @@ -1753,8 +1776,7 @@ static bool bifurcate(const Token* tok, const std::set& varids, cons return false; if (Token::Match(start, "; %varid% =", var->declarationId())) start = start->tokAt(2); - if (var->isConst() || - !isVariableChanged(start->next(), tok, var->declarationId(), var->isGlobal(), settings, true)) + if (var->isConst() || !bifurcateVariableChanged(var, varids, start, tok, settings, depth)) return var->isArgument() || bifurcate(start->astOperand2(), varids, settings, depth - 1); return false; } @@ -1949,7 +1971,7 @@ struct ValueFlowAnalyzer : Analyzer { if (isModified(tok).isModified()) a = Action::Invalid; if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok)) - a |= Action::Read; + a |= Action::Invalid; return a; } return Action::None; diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 77dbb8768..755ef4f5a 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4440,6 +4440,32 @@ private: " return static_cast(c);\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f(int x)\n" + "{\n" + " int i;\n" + " char value;\n" + " for(i = 0; i < 1; i++) {\n" + " if(x > 1)\n" + " value = 0;\n" + " }\n" + " printf(\"\", value);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:9]: (error) Uninitialized variable: value\n", errout.str()); + + valueFlowUninit("void f(int x)\n" + "{\n" + " int i;\n" + " char value;\n" + " for(i = 0; i < 1; i++) {\n" + " if(x > 1)\n" + " value = 0;\n" + " else\n" + " value = 1;\n" + " }\n" + " printf(\"\", value);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void uninitvar_ipa() { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 768725187..d67fcf663 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -2125,7 +2125,22 @@ private: " }\n" " a = x;\n" // <- x can't be 0 "}\n"; - ASSERT_EQUALS(false, testValueOfX(code, 9U, 0)); // x can't be 0 at line 9 + ASSERT_EQUALS(true, testValueOfX(code, 9U, 0)); // x can be 0 at line 9 + ASSERT_EQUALS(false, testValueOfXKnown(code, 9U, 0)); // x can't be known at line 9 + + code = "void f(const int *buf) {\n" + " int x = 0;\n" + " for (int i = 0; i < 10; i++) {\n" + " if (buf[i] == 123) {\n" + " x = i;\n" + " ;\n" // <- no break + " } else {\n" + " x = 1;\n" + " }\n" + " }\n" + " a = x;\n" // <- x can't be 0 + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 11U, 0)); // x can't be 0 at line 11 code = "void f(const int *buf) {\n" " int x = 0;\n"