walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix 10294: ValueFlow: Wrong <Uninit> value below loop (#3291)

This commit is contained in:
Paul Fultz II 2021-06-09 02:20:43 -05:00 committed by GitHub
parent 1e78b30958
commit f3a33ea330
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 248 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
lib/analyzer.h
View File

@ -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<int> evaluate(const Token* tok) const = 0;
virtual std::vector<int> 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<Analyzer> reanalyze(Token* tok, const std::string& msg = "") const = 0;
virtual ~Analyzer() {}

182
lib/forwardanalyzer.cpp
View File

@ -8,6 +8,8 @@
#include <algorithm>
#include <functional>
#include <tuple>
#include <utility>
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<bool, bool> evalCond(const Token* tok) {
std::vector<int> result = analyzer->evaluate(tok);
std::pair<bool, bool> evalCond(const Token* tok, const Token* ctx = nullptr) const {
if (!tok)
return std::make_pair(false, false);
std::vector<int> 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<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"))
@ -183,6 +193,7 @@ struct ForwardTraversal {
}
Progress updateRecursive(Token* tok) {
forked = false;
std::function<Progress(Token*)> 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<ForwardTraversal> tryForkScope(Token* endBlock, bool isModified = false) {
if (analyzer->updateScope(endBlock, isModified)) {
ForwardTraversal ft = forkScope(endBlock);
ForwardTraversal ft = fork();
return {ft};
}
return std::vector<ForwardTraversal> {};
}
std::vector<ForwardTraversal> tryForkUpdateScope(Token* endBlock, bool isModified = false)
{
std::vector<ForwardTraversal> 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<ForwardTraversal> ft1 = tryForkScope(endBlock, a.isModified());
bool bail = hasGoto(endBlock);
std::vector<ForwardTraversal> 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<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);
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<const Token*, const Token*> 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<ForwardTraversal> 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<ForwardTraversal> 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();

28
lib/programmemory.cpp
View File

@ -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;
}

3
lib/programmemory.h
View File

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

4
lib/reverseanalyzer.cpp
View File

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

32
lib/valueflow.cpp
View File

@ -236,6 +236,12 @@ const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, Val
if (tok->isComparisonOp()) {
std::vector<MathLib::bigint> value1 = evaluate(tok->astOperand1());
std::vector<MathLib::bigint> 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<int> evaluate(const Token* tok) const OVERRIDE {
virtual std::vector<int> evaluate(const Token* tok, const Token* ctx = nullptr) const OVERRIDE {
if (tok->hasKnownIntValue())
return {static_cast<int>(tok->values().front().intvalue)};
std::vector<int> 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();
}

3
test/testnullpointer.cpp
View File

@ -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];

66
test/testvalueflow.cpp
View File

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