From f3a33ea3303bc83760961e94f59efd5c8226ac1c Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 9 Jun 2021 02:20:43 -0500 Subject: [PATCH] Fix 10294: ValueFlow: Wrong value below loop (#3291) --- lib/analyzer.h | 12 ++- lib/forwardanalyzer.cpp | 182 ++++++++++++++++++++++++++------------- lib/programmemory.cpp | 28 ++++-- lib/programmemory.h | 3 +- lib/reverseanalyzer.cpp | 4 +- lib/valueflow.cpp | 32 +++++-- test/testnullpointer.cpp | 3 +- test/testvalueflow.cpp | 66 +++++++++++++- 8 files changed, 248 insertions(+), 82 deletions(-) diff --git a/lib/analyzer.h b/lib/analyzer.h index b278de4fa..33fc71883 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -113,12 +113,20 @@ struct Analyzer { enum class Direction { Forward, Reverse }; + struct Assume { + enum Flags { + None = 0, + Quiet = (1 << 0), + Absolute = (1 << 1), + }; + }; + /// Analyze a token virtual Action analyze(const Token* tok, Direction d) const = 0; /// Update the state of the value virtual void update(Token* tok, Action a, Direction d) = 0; /// Try to evaluate the value of a token(most likely a condition) - virtual std::vector evaluate(const Token* tok) const = 0; + virtual std::vector evaluate(const Token* tok, const Token* ctx = nullptr) const = 0; /// Lower any values to possible virtual bool lowerToPossible() = 0; /// Lower any values to inconclusive @@ -130,7 +138,7 @@ struct Analyzer { /// If the value is conditional virtual bool isConditional() const = 0; /// The condition that will be assumed during analysis - virtual void assume(const Token* tok, bool state, const Token* at = nullptr) = 0; + virtual void assume(const Token* tok, bool state, unsigned int flags = 0) = 0; /// Return analyzer for expression at token virtual ValuePtr reanalyze(Token* tok, const std::string& msg = "") const = 0; virtual ~Analyzer() {} diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index ecc6d4573..f9ba7e6a1 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -8,6 +8,8 @@ #include #include +#include +#include struct ForwardTraversal { enum class Progress { Continue, Break, Skip }; @@ -21,6 +23,7 @@ struct ForwardTraversal { bool analyzeOnly; bool analyzeTerminate; Terminate terminate = Terminate::None; + bool forked = false; Progress Break(Terminate t = Terminate::None) { if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None) @@ -29,11 +32,12 @@ struct ForwardTraversal { } struct Branch { + Branch(Token* tok = nullptr) : endBlock(tok) {} + Token* endBlock = nullptr; Analyzer::Action action = Analyzer::Action::None; bool check = false; bool escape = false; bool escapeUnknown = false; - const Token* endBlock = nullptr; bool isEscape() const { return escape || escapeUnknown; } @@ -56,8 +60,10 @@ struct ForwardTraversal { return actions.isModified(); } - std::pair evalCond(const Token* tok) { - std::vector result = analyzer->evaluate(tok); + std::pair evalCond(const Token* tok, const Token* ctx = nullptr) const { + if (!tok) + return std::make_pair(false, false); + std::vector result = analyzer->evaluate(tok, ctx); // TODO: We should convert to bool bool checkThen = std::any_of(result.begin(), result.end(), [](int x) { return x == 1; @@ -68,6 +74,10 @@ struct ForwardTraversal { return std::make_pair(checkThen, checkElse); } + bool isConditionTrue(const Token* tok, const Token* ctx = nullptr) const { return evalCond(tok, ctx).first; } + + bool isConditionFalse(const Token* tok, const Token* ctx = nullptr) const { return evalCond(tok, ctx).second; } + template)> Progress traverseTok(T* tok, std::function f, bool traverseUnknown, T** out = nullptr) { if (Token::Match(tok, "asm|goto|continue|setjmp|longjmp")) @@ -183,6 +193,7 @@ struct ForwardTraversal { } Progress updateRecursive(Token* tok) { + forked = false; std::function f = [this](Token* tok2) { return update(tok2); }; @@ -222,30 +233,33 @@ struct ForwardTraversal { return result; } - void forkRange(Token* start, const Token* end) { + ForwardTraversal fork(bool analyze = false) const { ForwardTraversal ft = *this; - ft.updateRange(start, end); - } - - 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); + ft.actions = Analyzer::Action::None; + ft.forked = true; return ft; } std::vector tryForkScope(Token* endBlock, bool isModified = false) { if (analyzer->updateScope(endBlock, isModified)) { - ForwardTraversal ft = forkScope(endBlock); + ForwardTraversal ft = fork(); return {ft}; } return std::vector {}; } + std::vector tryForkUpdateScope(Token* endBlock, bool isModified = false) + { + std::vector result = tryForkScope(endBlock, isModified); + for (ForwardTraversal& ft : result) + ft.updateScope(endBlock); + return result; + } + static bool hasGoto(const Token* endBlock) { return Token::findsimplematch(endBlock->link(), "goto", endBlock); } @@ -283,7 +297,7 @@ struct ForwardTraversal { Analyzer::Action checkScope(Token* endBlock) { Analyzer::Action a = analyzeScope(endBlock); - tryForkScope(endBlock, a.isModified()); + tryForkUpdateScope(endBlock, a.isModified()); return a; } @@ -292,16 +306,17 @@ struct ForwardTraversal { return a; } - bool checkBranch(Branch& branch, Token* endBlock) { - Analyzer::Action a = analyzeScope(endBlock); + bool checkBranch(Branch& branch) { + Analyzer::Action a = analyzeScope(branch.endBlock); branch.action = a; - std::vector ft1 = tryForkScope(endBlock, a.isModified()); - bool bail = hasGoto(endBlock); + std::vector ft1 = tryForkUpdateScope(branch.endBlock, a.isModified()); + bool bail = hasGoto(branch.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 (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) { + ForwardTraversal ft2 = fork(true); + ft2.updateScope(branch.endBlock); if (ft2.terminate == Terminate::Escape) { branch.escape = true; branch.escapeUnknown = false; @@ -317,12 +332,37 @@ struct ForwardTraversal { return bail; } - 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); + bool reentersLoop(Token* endBlock, const Token* condTok, const Token* stepTok) { + if (!condTok) + return true; + if (Token::simpleMatch(condTok, ":")) + return true; + bool changed = false; + if (stepTok) { + std::pair exprToks = stepTok->findExpressionStartEndTokens(); + if (exprToks.first != nullptr && exprToks.second != nullptr) + changed |= isExpressionChanged(condTok, exprToks.first, exprToks.second->next(), settings, true); } + changed |= isExpressionChanged(condTok, endBlock->link(), endBlock, settings, true); + // Check for mutation in the condition + changed |= nullptr != + findAstNode(condTok, [&](const Token* tok) { return isVariableChanged(tok, 0, settings, true); }); + if (!changed) + return true; + ForwardTraversal ft = fork(true); + ft.analyzer->assume(condTok, false, Analyzer::Assume::Absolute); + ft.updateScope(endBlock); + return ft.isConditionTrue(condTok); + } + + Progress updateInnerLoop(Token* endBlock, Token* stepTok, Token* condTok) { + if (endBlock && updateScope(endBlock) == Progress::Break) + return Break(); + if (stepTok && updateRecursive(stepTok) == Progress::Break) + return Break(); + if (condTok && updateRecursive(condTok) == Progress::Break) + return Break(); + return Progress::Continue; } Progress updateLoop(const Token* endToken, @@ -330,14 +370,13 @@ struct ForwardTraversal { Token* condTok, Token* initTok = nullptr, Token* stepTok = nullptr) { + if (initTok && updateRecursive(initTok) == Progress::Break) + return Break(); const bool isDoWhile = precedes(endBlock, condTok); - const bool alwaysEnterLoop = !condTok || (condTok->hasKnownIntValue() && condTok->values().front().intvalue != 0); Analyzer::Action bodyAnalysis = analyzeScope(endBlock); Analyzer::Action allAnalysis = bodyAnalysis; if (condTok) allAnalysis |= analyzeRecursive(condTok); - if (initTok) - allAnalysis |= analyzeRecursive(initTok); if (stepTok) allAnalysis |= analyzeRecursive(stepTok); actions |= allAnalysis; @@ -348,43 +387,64 @@ struct ForwardTraversal { if (!analyzer->lowerToPossible()) return Break(Terminate::Bail); } - // Traverse condition after lowering - if (condTok && (!isDoWhile || (!bodyAnalysis.isModified() && !bodyAnalysis.isIdempotent()))) { - if (updateRecursive(condTok) == Progress::Break) + bool checkThen = true; + bool checkElse = false; + if (condTok && !Token::simpleMatch(condTok, ":")) { + if (!isDoWhile || (!bodyAnalysis.isModified() && !bodyAnalysis.isIdempotent())) + if (updateRecursive(condTok) == Progress::Break) + return Break(); + // TODO: Check cond first before lowering + std::tie(checkThen, checkElse) = evalCond(condTok, isDoWhile ? endBlock->link()->previous() : nullptr); + } + // condition is false, we don't enter the loop + if (checkElse) + return Progress::Continue; + if (checkThen || isDoWhile) { + if (updateInnerLoop(endBlock, stepTok, condTok) == Progress::Break) return Break(); - - bool checkThen = true; - bool checkElse = false; - std::tie(checkThen, checkElse) = evalCond(condTok); - // condition is false, we don't enter the loop - if (checkElse) - return Progress::Continue; - } - if (allAnalysis.isModified() && alwaysEnterLoop) - return Break(Terminate::Bail); - - 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); - if (!Token::Match(nextStatement, ";|} break ;")) { - if (!allAnalysis.isIncremental()) - continueUpdateRangeAfterLoop(ftv, endBlock, endToken); + // If loop re-enters then it could be modified again + if (allAnalysis.isModified() && reentersLoop(endBlock, condTok, stepTok)) + return Break(Terminate::Bail); + if (allAnalysis.isIncremental()) return Break(Terminate::Bail); - } } else { - if (stepTok && updateRecursive(stepTok) == Progress::Break) { - if (!allAnalysis.isIncremental()) - continueUpdateRangeAfterLoop(ftv, endBlock, endToken); - return Break(Terminate::Bail); + std::vector ftv = tryForkScope(endBlock, allAnalysis.isModified()); + bool forkContinue = true; + for (ForwardTraversal& ft : ftv) { + if (condTok) + ft.analyzer->assume(condTok, false, Analyzer::Assume::Quiet); + if (ft.updateInnerLoop(endBlock, stepTok, condTok) == Progress::Break) + forkContinue = false; } + + if (allAnalysis.isModified() || !forkContinue) { + // TODO: Dont bail on missing condition + if (!condTok) + return Break(Terminate::Bail); + if (analyzer->isConditional() && stopUpdates()) + return Break(Terminate::Conditional); + analyzer->assume(condTok, false); + } + if (forkContinue) { + for (ForwardTraversal& ft : ftv) { + if (!ft.actions.isIncremental()) + ft.updateRange(endBlock, endToken); + } + } + if (allAnalysis.isIncremental()) + return Break(Terminate::Bail); } - // TODO: Should we traverse the body? - // updateRange(endBlock->link(), endBlock); return Progress::Continue; } + Progress updateScope(Token* endBlock) { + if (forked) + analyzer->forkScope(endBlock); + return updateRange(endBlock->link(), endBlock); + } + Progress updateRange(Token* start, const Token* end, int depth = 20) { + forked = false; if (depth < 0) return Break(Terminate::Bail); std::size_t i = 0; @@ -454,7 +514,7 @@ struct ForwardTraversal { if (updateRecursive(condTok) == Progress::Break) return Break(); } - analyzer->assume(condTok, !inElse, tok); + analyzer->assume(condTok, !inElse, Analyzer::Assume::Quiet); if (Token::simpleMatch(tok, "} else {")) tok = tok->linkAt(2); } else if (scope->type == Scope::eTry) { @@ -494,8 +554,8 @@ struct ForwardTraversal { // Traverse condition if (updateRecursive(condTok) == Progress::Break) return Break(); - Branch thenBranch{}; - Branch elseBranch{}; + Branch thenBranch{endBlock}; + Branch elseBranch{endBlock->tokAt(2) ? endBlock->linkAt(2) : nullptr}; // Check if condition is true or false std::tie(thenBranch.check, elseBranch.check) = evalCond(condTok); bool hasElse = Token::simpleMatch(endBlock, "} else {"); @@ -507,7 +567,7 @@ struct ForwardTraversal { if (updateRange(endCond->next(), endBlock, depth - 1) == Progress::Break) return Break(); } else if (!elseBranch.check) { - if (checkBranch(thenBranch, endBlock)) + if (checkBranch(thenBranch)) bail = true; } // Traverse else block @@ -518,7 +578,7 @@ struct ForwardTraversal { if (result == Progress::Break) return Break(); } else if (!thenBranch.check) { - if (checkBranch(elseBranch, endBlock->linkAt(2))) + if (checkBranch(elseBranch)) bail = true; } tok = endBlock->linkAt(2); @@ -583,7 +643,7 @@ struct ForwardTraversal { if (checkElse) return Break(); if (!checkThen) - analyzer->assume(condTok, true, tok); + analyzer->assume(condTok, true, Analyzer::Assume::Quiet | Analyzer::Assume::Absolute); } else if (Token::simpleMatch(tok, "switch (")) { if (updateRecursive(tok->next()->astOperand2()) == Progress::Break) return Break(); diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 5dfd081bb..9905bb074 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -199,7 +199,11 @@ static void fillProgramMemoryFromConditions(ProgramMemory& pm, const Scope* scop const Token* condTok = getCondTokFromEnd(scope->bodyEnd); if (!condTok) return; - programMemoryParseCondition(pm, condTok, endTok, settings, scope->type != Scope::eElse); + MathLib::bigint result = 0; + bool error = false; + execute(condTok, &pm, &result, &error); + if (error) + programMemoryParseCondition(pm, condTok, endTok, settings, scope->type != Scope::eElse); } } @@ -321,7 +325,11 @@ void ProgramMemoryState::assume(const Token* tok, bool b) { ProgramMemory pm = state; programMemoryParseCondition(pm, tok, nullptr, nullptr, b); - insert(pm, tok); + const Token* origin = tok; + const Token* top = tok->astTop(); + if (top && Token::Match(top->previous(), "for|while (")) + origin = top->link(); + replace(pm, origin); } void ProgramMemoryState::removeModifiedVars(const Token* tok) @@ -336,11 +344,21 @@ void ProgramMemoryState::removeModifiedVars(const Token* tok) } } -ProgramMemory ProgramMemoryState::get(const Token *tok, const ProgramMemory::Map& vars) const +ProgramMemory ProgramMemoryState::get(const Token* tok, const Token* ctx, const ProgramMemory::Map& vars) const { ProgramMemoryState local = *this; - local.addState(tok, vars); - local.removeModifiedVars(tok); + if (ctx) + local.addState(ctx, vars); + const Token* start = previousBeforeAstLeftmostLeaf(tok); + if (!start) + start = tok; + + if (!ctx || precedes(start, ctx)) { + local.addState(start, vars); + local.removeModifiedVars(start); + } else { + local.removeModifiedVars(ctx); + } return local.state; } diff --git a/lib/programmemory.h b/lib/programmemory.h index ece9a4399..7ae865967 100644 --- a/lib/programmemory.h +++ b/lib/programmemory.h @@ -53,8 +53,7 @@ struct ProgramMemoryState { void removeModifiedVars(const Token* tok); - ProgramMemory get(const Token *tok, const ProgramMemory::Map& vars) const; - + ProgramMemory get(const Token* tok, const Token* ctx, const ProgramMemory::Map& vars) const; }; void execute(const Token *expr, diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index 69765dc6b..31873119b 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -218,9 +218,9 @@ struct ReverseTraversal { if (thenAction.isModified() && inLoop) break; else if (thenAction.isModified() && !elseAction.isModified()) - analyzer->assume(condTok, hasElse, condTok); + analyzer->assume(condTok, hasElse); else if (elseAction.isModified() && !thenAction.isModified()) - analyzer->assume(condTok, !hasElse, condTok); + analyzer->assume(condTok, !hasElse); // Bail if one of the branches are read to avoid FPs due to over constraints else if (thenAction.isIdempotent() || elseAction.isIdempotent() || thenAction.isRead() || elseAction.isRead()) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 443de6266..a030f5cbd 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -236,6 +236,12 @@ const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, Val if (tok->isComparisonOp()) { std::vector value1 = evaluate(tok->astOperand1()); std::vector value2 = evaluate(tok->astOperand2()); + if (!value1.empty() && !value2.empty()) { + if (tok->astOperand1()->hasKnownIntValue()) + value2.clear(); + if (tok->astOperand2()->hasKnownIntValue()) + value1.clear(); + } if (!value1.empty()) { if (isSaturated(value1.front())) return nullptr; @@ -2158,11 +2164,11 @@ struct ValueFlowAnalyzer : Analyzer { return Action::None; } - virtual std::vector evaluate(const Token* tok) const OVERRIDE { + virtual std::vector evaluate(const Token* tok, const Token* ctx = nullptr) const OVERRIDE { if (tok->hasKnownIntValue()) return {static_cast(tok->values().front().intvalue)}; std::vector result; - ProgramMemory pm = pms.get(tok, getProgramState()); + ProgramMemory pm = pms.get(tok, ctx, getProgramState()); if (Token::Match(tok, "&&|%oror%")) { if (conditionIsTrue(tok, pm)) result.push_back(1); @@ -2179,19 +2185,33 @@ struct ValueFlowAnalyzer : Analyzer { return result; } - virtual void assume(const Token* tok, bool state, const Token* at) OVERRIDE { + virtual void assume(const Token* tok, bool state, unsigned int flags) OVERRIDE { // Update program state pms.removeModifiedVars(tok); pms.addState(tok, getProgramState()); pms.assume(tok, state); - const bool isAssert = Token::Match(at, "assert|ASSERT"); + bool isCondBlock = false; + const Token* parent = tok->astParent(); + if (parent) { + isCondBlock = Token::Match(parent->previous(), "if|while ("); + } - if (!isAssert && !Token::simpleMatch(at, "}")) { + if (isCondBlock) { + const Token* startBlock = parent->link()->next(); + const Token* endBlock = startBlock->link(); + pms.removeModifiedVars(endBlock); + if (state) + pms.addState(endBlock->previous(), getProgramState()); + else if (Token::simpleMatch(endBlock, "} else {")) + pms.addState(endBlock->linkAt(2)->previous(), getProgramState()); + } + + if (!(flags & Assume::Quiet)) { std::string s = state ? "true" : "false"; addErrorPath(tok, "Assuming condition is " + s); } - if (!isAssert) + if (!(flags & Assume::Absolute)) makeConditional(); } diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index a14a3b096..3ad9692b7 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -1218,8 +1218,7 @@ private: " argv32[i] = 0;\n" " }\n" "}"); - TODO_ASSERT_EQUALS("error", - "", errout.str()); + ASSERT_EQUALS("[test.cpp:10]: (warning) Possible null pointer dereference: argv32\n", errout.str()); // #2231 - error if assignment in loop is not used // extracttests.start: int y[20]; diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 772b4e85a..00b5dcba1 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -2151,6 +2151,7 @@ private: ASSERT_EQUALS(false, testValueOfX(code, 11U, 0)); // x can't be 0 at line 11 code = "void f(const int *buf) {\n" + " int i = 0;\n" " int x = 0;\n" " while (++i < 10) {\n" " if (buf[i] == 123) {\n" @@ -2160,7 +2161,66 @@ private: " }\n" " a = x;\n" // <- x can be 0 "}\n"; - ASSERT_EQUALS(true, testValueOfX(code, 9U, 0)); // x can be 0 at line 9 + ASSERT_EQUALS(true, testValueOfX(code, 10U, 0)); // x can be 0 at line 9 + + code = "bool maybe();\n" + "void f() {\n" + " int x = 0;\n" + " bool found = false;\n" + " while(!found) {\n" + " if (maybe()) {\n" + " x = i;\n" + " found = true;\n" + " }\n" + " }\n" + " a = x;\n" // <- x can't be 0 + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 11U, 0)); + + code = "bool maybe();\n" + "void f() {\n" + " int x = 0;\n" + " bool found = false;\n" + " while(!found) {\n" + " if (maybe()) {\n" + " x = i;\n" + " found = true;\n" + " } else {\n" + " found = false;\n" + " }\n" + " }\n" + " a = x;\n" // <- x can't be 0 + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 13U, 0)); + + code = "bool maybe();\n" + "void f() {\n" + " int x = 0;\n" + " bool found = false;\n" + " while(!found) {\n" + " if (maybe()) {\n" + " x = i;\n" + " break;\n" + " }\n" + " }\n" + " a = x;\n" // <- x can't be 0 + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 11U, 0)); + + code = "bool maybe();\n" + "void f() {\n" + " int x = 0;\n" + " bool found = false;\n" + " while(!found) {\n" + " if (maybe()) {\n" + " x = i;\n" + " found = true;\n" + " break;\n" + " }\n" + " }\n" + " a = x;\n" // <- x can't be 0 + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 12U, 0)); code = "void f(const int a[]) {\n" // #6616 " const int *x = 0;\n" @@ -3176,7 +3236,9 @@ private: " } while (condition)\n" "}"; values = tokenValues(code, "=="); - ASSERT_EQUALS(true, values.empty()); + ASSERT_EQUALS(1, values.size()); + ASSERT(values.front().isPossible()); + ASSERT_EQUALS(1, values.front().intvalue); // for loops code = "struct S { int x; };\n" // #9036