Fix issue 9030: ValueFlow: Possible value after conditional assignment in for loop (#3059)

This commit is contained in:
Paul Fultz II 2021-01-18 03:12:07 -06:00 committed by GitHub
parent 664289792c
commit 25ada657da
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 182 additions and 83 deletions

View File

@ -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>& 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<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*>)>
Progress traverseTok(T* tok, std::function<Progress(T*)> 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<Progress(const Token*)> 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<ForwardTraversal> 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<ForwardTraversal>{};
}
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<ForwardTraversal>& 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);
if (checkElse)
// condition is false, we don't enter the loop
return Progress::Break;
if (checkElse)
return Progress::Continue;
}
forkScope(endBlock, allAnalysis.isModified());
std::vector<ForwardTraversal> 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

View File

@ -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();

View File

@ -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<nonneg int>& varids, const Settings* settings, int depth = 20)
static bool bifurcate(const Token* tok, const std::set<nonneg int>& varids, const Settings* settings, int depth = 20);
static bool bifurcateVariableChanged(const Variable* var,
const std::set<nonneg int>& 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<nonneg int>& varids, const Settings* settings, int depth)
{
if (depth < 0)
return false;
@ -1753,8 +1776,7 @@ static bool bifurcate(const Token* tok, const std::set<nonneg int>& 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;

View File

@ -4440,6 +4440,32 @@ private:
" return static_cast<char>(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() {

View File

@ -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"