From d7c914bd3e484318fb18e69c911861e1f45c672f Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 29 May 2022 12:57:10 -0500 Subject: [PATCH] Handle subfunction values in valueflow conditions (#4128) --- lib/analyzer.h | 2 - lib/forwardanalyzer.cpp | 6 -- lib/valueflow.cpp | 232 ++++++++++++++++++++++++++++------------ test/testvalueflow.cpp | 25 +++++ 4 files changed, 189 insertions(+), 76 deletions(-) diff --git a/lib/analyzer.h b/lib/analyzer.h index d28bf039b..2ab5544a3 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -172,8 +172,6 @@ struct Analyzer { virtual bool lowerToInconclusive() = 0; /// If the analysis is unsure whether to update a scope, this will return true if the analysis should bifurcate the scope virtual bool updateScope(const Token* endBlock, bool modified) const = 0; - /// Called when a scope will be forked - virtual void forkScope(const Token* /*endBlock*/) {} /// If the value is conditional virtual bool isConditional() const = 0; /// If analysis should stop on the condition diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 07460af58..a00b1e363 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -59,7 +59,6 @@ struct ForwardTraversal { bool analyzeOnly; bool analyzeTerminate; Analyzer::Terminate terminate = Analyzer::Terminate::None; - bool forked = false; std::vector loopEnds = {}; Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) { @@ -250,7 +249,6 @@ struct ForwardTraversal { } Progress updateRecursive(Token* tok) { - forked = false; auto f = [this](Token* tok2) { return update(tok2); }; @@ -297,7 +295,6 @@ struct ForwardTraversal { ft.analyzeTerminate = true; } ft.actions = Analyzer::Action::None; - ft.forked = true; return ft; } @@ -545,13 +542,10 @@ struct ForwardTraversal { } 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(Analyzer::Terminate::Bail); std::size_t i = 0; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 508f23005..51a3daf3a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -174,6 +174,19 @@ static void changePossibleToKnown(std::list& values, int indir } } +static bool isNonConditionalPossibleIntValue(const ValueFlow::Value& v) +{ + if (v.conditional) + return false; + if (v.condition) + return false; + if (!v.isPossible()) + return false; + if (!v.isIntValue()) + return false; + return true; +} + static void setValueUpperBound(ValueFlow::Value& value, bool upper) { if (upper) @@ -191,6 +204,14 @@ static void setValueBound(ValueFlow::Value& value, const Token* tok, bool invert } } +static void setConditionalValue(ValueFlow::Value& value, const Token* tok, MathLib::bigint i) +{ + assert(value.isIntValue()); + value.intvalue = i; + value.assumeCondition(tok); + value.setPossible(); +} + static void setConditionalValues(const Token* tok, bool lhs, MathLib::bigint value, @@ -216,11 +237,11 @@ static void setConditionalValues(const Token* tok, if (lhs) std::swap(greaterThan, lessThan); if (Token::simpleMatch(tok, greaterThan, strlen(greaterThan))) { - true_value = ValueFlow::Value{tok, value + 1}; - false_value = ValueFlow::Value{tok, value}; + setConditionalValue(true_value, tok, value + 1); + setConditionalValue(false_value, tok, value); } else if (Token::simpleMatch(tok, lessThan, strlen(lessThan))) { - true_value = ValueFlow::Value{tok, value - 1}; - false_value = ValueFlow::Value{tok, value}; + setConditionalValue(true_value, tok, value - 1); + setConditionalValue(false_value, tok, value); } } setValueBound(true_value, tok, lhs); @@ -232,32 +253,85 @@ static bool isSaturated(MathLib::bigint value) return value == std::numeric_limits::max() || value == std::numeric_limits::min(); } -const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value, const std::function(const Token*)>& evaluate) +static void parseCompareEachInt( + const Token* tok, + const std::function& each, + const std::function(const Token*)>& evaluate) { if (!tok->astOperand1() || !tok->astOperand2()) - return nullptr; + return; if (tok->isComparisonOp()) { - std::vector value1 = evaluate(tok->astOperand1()); - std::vector value2 = evaluate(tok->astOperand2()); + 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()) || astIsFloat(tok->astOperand2(), /*unknown*/ false)) - return nullptr; - setConditionalValues(tok, true, value1.front(), true_value, false_value); - return tok->astOperand2(); - } else if (!value2.empty()) { - if (isSaturated(value2.front()) || astIsFloat(tok->astOperand1(), /*unknown*/ false)) - return nullptr; - setConditionalValues(tok, false, value2.front(), true_value, false_value); - return tok->astOperand1(); + for (const ValueFlow::Value& v1 : value1) { + ValueFlow::Value true_value = v1; + ValueFlow::Value false_value = v1; + if (isSaturated(v1.intvalue) || astIsFloat(tok->astOperand2(), /*unknown*/ false)) + continue; + setConditionalValues(tok, true, v1.intvalue, true_value, false_value); + each(tok->astOperand2(), std::move(true_value), std::move(false_value)); + } + for (const ValueFlow::Value& v2 : value2) { + ValueFlow::Value true_value = v2; + ValueFlow::Value false_value = v2; + if (isSaturated(v2.intvalue) || astIsFloat(tok->astOperand1(), /*unknown*/ false)) + continue; + setConditionalValues(tok, false, v2.intvalue, true_value, false_value); + each(tok->astOperand1(), std::move(true_value), std::move(false_value)); } } - return nullptr; +} + +static void parseCompareEachInt( + const Token* tok, + const std::function& each) +{ + parseCompareEachInt(tok, each, [](const Token* t) -> std::vector { + if (t->hasKnownIntValue()) + return {t->values().front()}; + std::vector result; + std::copy_if(t->values().begin(), t->values().end(), std::back_inserter(result), [&](const ValueFlow::Value& v) { + if (v.path < 1) + return false; + if (!isNonConditionalPossibleIntValue(v)) + return false; + return true; + }); + return result; + }); +} + +const Token* parseCompareInt(const Token* tok, + ValueFlow::Value& true_value, + ValueFlow::Value& false_value, + const std::function(const Token*)>& evaluate) +{ + const Token* result = nullptr; + parseCompareEachInt( + tok, + [&](const Token* vartok, ValueFlow::Value true_value2, ValueFlow::Value false_value2) { + if (result) + return; + result = vartok; + true_value = std::move(true_value2); + false_value = std::move(false_value2); + }, + [&](const Token* t) -> std::vector { + std::vector r; + std::vector v = evaluate(t); + + std::transform(v.begin(), v.end(), std::back_inserter(r), [&](MathLib::bigint i) { + return ValueFlow::Value{i}; + }); + return r; + }); + return result; } const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value) @@ -5431,6 +5505,25 @@ struct ConditionHandler { return astIsBool(vartok); } + static MathLib::bigint findPath(const std::list& values) + { + auto it = std::find_if(values.begin(), values.end(), [](const ValueFlow::Value& v) { + return v.path > 0; + }); + if (it == values.end()) + return 0; + assert(std::all_of(it, values.end(), [&](const ValueFlow::Value& v) { + return v.path == 0 || v.path == it->path; + })); + return it->path; + } + + MathLib::bigint getPath() const + { + assert(std::abs(findPath(true_values) - findPath(false_values)) == 0); + return findPath(true_values) | findPath(false_values); + } + Condition() : vartok(nullptr), true_values(), false_values(), inverted(false), impossible(true) {} }; @@ -5628,6 +5721,21 @@ struct ConditionHandler { return tok; } + static void fillFromPath(ProgramMemory& pm, const Token* top, MathLib::bigint path) + { + if (path < 1) + return; + visitAstNodes(top, [&](const Token* tok) { + const ValueFlow::Value* v = ValueFlow::findValue(tok->values(), nullptr, [&](const ValueFlow::Value& v) { + return v.path == path && isNonConditionalPossibleIntValue(v); + }); + if (v == nullptr) + return ChildrenToVisit::op1_and_op2; + pm.setValue(tok, *v); + return ChildrenToVisit::op1_and_op2; + }); + } + void afterCondition(TokenList* tokenlist, SymbolDatabase* symboldatabase, ErrorLogger* errorLogger, @@ -5635,17 +5743,21 @@ struct ConditionHandler { traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* condTok, const Scope* scope) { const Token* top = condTok->astTop(); + const MathLib::bigint path = cond.getPath(); + const bool allowKnown = path == 0; + const bool allowImpossible = cond.impossible && allowKnown; + std::list thenValues; std::list elseValues; if (!Token::Match(condTok, "!=|=|(|.") && condTok != cond.vartok) { thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end()); - if (cond.impossible && isConditionKnown(condTok, false)) + if (allowImpossible && isConditionKnown(condTok, false)) insertImpossible(elseValues, cond.false_values); } if (!Token::Match(condTok, "==|!")) { elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end()); - if (cond.impossible && isConditionKnown(condTok, true)) { + if (allowImpossible && isConditionKnown(condTok, true)) { insertImpossible(thenValues, cond.true_values); if (cond.isBool()) insertNegateKnown(thenValues, cond.true_values); @@ -5676,7 +5788,7 @@ struct ConditionHandler { values = thenValues; else if (op == "||") values = elseValues; - if (Token::Match(condTok, "==|!=") || cond.isBool()) + if (allowKnown && (Token::Match(condTok, "==|!=") || cond.isBool())) changePossibleToKnown(values); if (astIsFloat(cond.vartok, false) || (!cond.vartok->valueType() && @@ -5799,6 +5911,8 @@ struct ConditionHandler { // Check if condition in for loop is always false const Token* initTok = getInitTok(top); ProgramMemory pm; + fillFromPath(pm, initTok, path); + fillFromPath(pm, condTok, path); execute(initTok, &pm, nullptr, nullptr); MathLib::bigint result = 1; execute(condTok, &pm, &result, nullptr); @@ -5830,7 +5944,8 @@ struct ConditionHandler { if (!startToken) continue; std::list& values = (i == 0 ? thenValues : elseValues); - valueFlowSetConditionToKnown(condTok, values, i == 0); + if (allowKnown) + valueFlowSetConditionToKnown(condTok, values, i == 0); Analyzer::Result r = forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist); deadBranch[i] = r.terminate == Analyzer::Terminate::Escape; @@ -5927,7 +6042,7 @@ struct ConditionHandler { if (possible) { values.remove_if(std::mem_fn(&ValueFlow::Value::isImpossible)); changeKnownToPossible(values); - } else { + } else if (allowKnown) { valueFlowSetConditionToKnown(condTok, values, true); valueFlowSetConditionToKnown(condTok, values, false); } @@ -5953,20 +6068,26 @@ static void valueFlowCondition(const ValuePtr& handler, struct SimpleConditionHandler : ConditionHandler { virtual std::vector parse(const Token* tok, const Settings*) const override { - Condition cond; - ValueFlow::Value true_value; - ValueFlow::Value false_value; - const Token *vartok = parseCompareInt(tok, true_value, false_value); - if (vartok) { + + std::vector conds; + parseCompareEachInt(tok, [&](const Token* vartok, ValueFlow::Value true_value, ValueFlow::Value false_value) { + Condition cond; if (vartok->hasKnownIntValue()) - return {}; + return; if (vartok->str() == "=" && vartok->astOperand1() && vartok->astOperand2()) vartok = vartok->astOperand1(); cond.true_values.push_back(true_value); cond.false_values.push_back(false_value); cond.vartok = vartok; - return {cond}; - } + conds.push_back(cond); + }); + if (!conds.empty()) + return conds; + + Condition cond; + ValueFlow::Value true_value; + ValueFlow::Value false_value; + const Token* vartok = nullptr; if (tok->str() == "!") { vartok = tok->astOperand1(); @@ -6562,34 +6683,6 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer { } return ps; } - - virtual void forkScope(const Token* endBlock) override { - ProgramMemory pm{getProgramState()}; - const Scope* scope = endBlock->scope(); - const Token* condTok = getCondTokFromEnd(endBlock); - if (scope && condTok) - programMemoryParseCondition(pm, condTok, nullptr, getSettings(), scope->type != Scope::eElse); - if (condTok && Token::simpleMatch(condTok->astParent(), ";")) { - ProgramMemory endMemory; - if (valueFlowForLoop2(condTok->astTop()->previous(), nullptr, &endMemory, nullptr)) - pm.replace(endMemory); - } - // ProgramMemory pm = pms.get(endBlock->link()->next(), getProgramState()); - for (const auto& p : pm) { - nonneg int varid = p.first.getExpressionId(); - if (symboldatabase && !symboldatabase->isVarId(varid)) - continue; - ValueFlow::Value value = p.second; - if (vars.count(varid) != 0) - continue; - if (value.isImpossible()) - continue; - value.setPossible(); - values[varid] = value; - if (symboldatabase) - vars[varid] = symboldatabase->getVariableFromVarId(varid); - } - } }; template @@ -7845,21 +7938,24 @@ static void valueFlowContainerSize(TokenList* tokenlist, struct ContainerConditionHandler : ConditionHandler { virtual std::vector parse(const Token* tok, const Settings* settings) const override { - Condition cond; - ValueFlow::Value true_value; - ValueFlow::Value false_value; - const Token *vartok = parseCompareInt(tok, true_value, false_value); - if (vartok) { + std::vector conds; + parseCompareEachInt(tok, [&](const Token* vartok, ValueFlow::Value true_value, ValueFlow::Value false_value) { + Condition cond; vartok = settings->library.getContainerFromYield(vartok, Library::Container::Yield::SIZE); if (!vartok) - return {}; + return; true_value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; false_value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; cond.true_values.push_back(true_value); cond.false_values.push_back(false_value); cond.vartok = vartok; - return {cond}; - } + conds.push_back(cond); + }); + if (!conds.empty()) + return conds; + + Condition cond; + const Token* vartok = nullptr; // Empty check if (tok->str() == "(") { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 2c8ad19cf..c842111a4 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4357,6 +4357,31 @@ private: " f(x, -1);\n" "}\n"; ASSERT_EQUALS(true, testValueOfX(code, 4U, -1)); + + code = "void g() {\n" + " const std::vector v;\n" + " f(v);\n" + "}\n" + "void f(const std::vector& w) {\n" + " for (int i = 0; i < w.size(); ++i) {\n" + " int x = i != 0;\n" + " int a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfXKnown(code, 8U, 0)); + ASSERT_EQUALS(false, testValueOfXKnown(code, 8U, 1)); + + code = "void g() {\n" + " const std::vector v;\n" + " f(v);\n" + "}\n" + "void f(const std::vector& w) {\n" + " for (int i = 0; i < w.size(); ++i) {\n" + " int x = i;\n" + " int a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 8U, -1)); } void valueFlowFunctionReturn() { const char *code;