Skip to content

Fix 10314: Possible nullPointerRedundantCheck false positive #3298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 7 commits into from
Jun 19, 2021
Merged

Fix 10314: Possible nullPointerRedundantCheck false positive #3298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
255680d
Fix 10314: Possible nullPointerRedundantCheck false positive
pfultz2 Jun 14, 2021
aa4f03b
Merge
pfultz2 Jun 14, 2021
e25f4c6
Format
pfultz2 Jun 14, 2021
d88e4d6
Init variable
pfultz2 Jun 15, 2021
6f98977
Default init class
pfultz2 Jun 15, 2021
d065016
Add constructor
pfultz2 Jun 16, 2021
da2b37d
Format
pfultz2 Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/analyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,16 @@ struct Analyzer {
unsigned int mFlag;
};

enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };

struct Result {
Result(Action action = Action::None, Terminate terminate = Terminate::None)
: action(action), terminate(terminate)
{}
Action action;
Terminate terminate;
};

enum class Direction { Forward, Reverse };

struct Assume {
Expand Down
3 changes: 2 additions & 1 deletion lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1563,7 +1563,8 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To
if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {"))
return isReturnScope(prev, library, unknownFunc, functionScope) &&
isReturnScope(prev->link()->tokAt(-2), library, unknownFunc, functionScope);
if (Token::simpleMatch(prev->link()->previous(), ") {") &&
// TODO: Check all cases
if (!functionScope && Token::simpleMatch(prev->link()->previous(), ") {") &&
Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") &&
!Token::findsimplematch(prev->link(), "break", prev)) {
return true;
Expand Down
74 changes: 36 additions & 38 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@ struct ForwardTraversal {
Analyzer::Action actions;
bool analyzeOnly;
bool analyzeTerminate;
Terminate terminate = Terminate::None;
Analyzer::Terminate terminate = Analyzer::Terminate::None;
bool forked = false;

Progress Break(Terminate t = Terminate::None) {
if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None)
Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None)
{
if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
terminate = t;
return Progress::Break;
}
Expand Down Expand Up @@ -89,7 +90,7 @@ struct ForwardTraversal {
else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
traverseRecursive(tok->astOperand1(), f, traverseUnknown);
traverseRecursive(tok->astOperand2(), f, traverseUnknown);
return Break(Terminate::Escape);
return Break(Analyzer::Terminate::Escape);
} else if (isUnevaluated(tok)) {
if (out)
*out = tok->link();
Expand All @@ -103,7 +104,7 @@ struct ForwardTraversal {
// Skip lambdas
} else if (T* lambdaEndToken = findLambdaEndToken(tok)) {
if (checkScope(lambdaEndToken).isModified())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (out)
*out = lambdaEndToken->next();
// Skip class scope
Expand Down Expand Up @@ -152,7 +153,7 @@ struct ForwardTraversal {
if (!checkThen && !checkElse) {
// Stop if the value is conditional
if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) {
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
}
checkThen = true;
checkElse = true;
Expand Down Expand Up @@ -180,12 +181,12 @@ struct ForwardTraversal {
if (!action.isNone() && !analyzeOnly)
analyzer->update(tok, action, Analyzer::Direction::Forward);
if (action.isInconclusive() && !analyzer->lowerToInconclusive())
return Break(Terminate::Inconclusive);
return Break(Analyzer::Terminate::Inconclusive);
if (action.isInvalid())
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
if (action.isWrite() && !action.isRead())
// Analysis of this write will continue separately
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
return Progress::Continue;
}

Expand Down Expand Up @@ -320,13 +321,13 @@ struct ForwardTraversal {
if (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) {
ForwardTraversal ft2 = fork(true);
ft2.updateScope(branch.endBlock);
if (ft2.terminate == Terminate::Escape) {
if (ft2.terminate == Analyzer::Terminate::Escape) {
branch.escape = true;
branch.escapeUnknown = false;
}
}
} else {
if (ft1.front().terminate == Terminate::Escape) {
if (ft1.front().terminate == Analyzer::Terminate::Escape) {
branch.escape = true;
branch.escapeUnknown = false;
}
Expand Down Expand Up @@ -387,10 +388,10 @@ struct ForwardTraversal {
actions |= allAnalysis;
if (allAnalysis.isInconclusive()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (allAnalysis.isModified()) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
}
bool checkThen = true;
bool checkElse = false;
Expand All @@ -409,9 +410,9 @@ struct ForwardTraversal {
return Break();
// If loop re-enters then it could be modified again
if (allAnalysis.isModified() && reentersLoop(endBlock, condTok, stepTok))
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (allAnalysis.isIncremental())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else {
std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
bool forkContinue = true;
Expand All @@ -425,9 +426,9 @@ struct ForwardTraversal {
if (allAnalysis.isModified() || !forkContinue) {
// TODO: Dont bail on missing condition
if (!condTok)
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
analyzer->assume(condTok, false);
}
if (forkContinue) {
Expand All @@ -437,7 +438,7 @@ struct ForwardTraversal {
}
}
if (allAnalysis.isIncremental())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
}
return Progress::Continue;
}
Expand All @@ -451,7 +452,7 @@ struct ForwardTraversal {
Progress updateRange(Token* start, const Token* end, int depth = 20) {
forked = false;
if (depth < 0)
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
std::size_t i = 0;
for (Token* tok = start; tok && tok != end; tok = tok->next()) {
Token* next = nullptr;
Expand Down Expand Up @@ -485,13 +486,13 @@ struct ForwardTraversal {
return Break();
tok = skipTo(tok, scopeEndToken, end);
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
// TODO: Don't break, instead move to the outer scope
if (!tok)
return Break();
} else if (Token::Match(tok, "%name% :") || tok->str() == "case") {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (tok->link() && tok->str() == "}") {
const Scope* scope = tok->scope();
if (!scope)
Expand All @@ -505,7 +506,7 @@ struct ForwardTraversal {
return Break();
if (!condTok->hasKnownIntValue() || inLoop) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (condTok->values().front().intvalue == inElse) {
return Break();
}
Expand All @@ -524,7 +525,7 @@ struct ForwardTraversal {
tok = tok->linkAt(2);
} else if (scope->type == Scope::eTry) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (scope->type == Scope::eLambda) {
return Break();
} else if (scope->type == Scope::eDo && Token::simpleMatch(tok, "} while (")) {
Expand Down Expand Up @@ -595,32 +596,32 @@ struct ForwardTraversal {
return Break();
if (thenBranch.isDead() && elseBranch.isDead()) {
if (thenBranch.isModified() && elseBranch.isModified())
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape())
return Break(Terminate::Escape);
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Escape);
return Break(Analyzer::Terminate::Bail);
}
// Conditional return
if (thenBranch.isEscape() && !hasElse) {
if (!thenBranch.isConclusiveEscape()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (thenBranch.check) {
return Break();
} else {
if (analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
analyzer->assume(condTok, false);
}
}
if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (thenBranch.isModified() || elseBranch.isModified()) {
if (!hasElse && analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
analyzer->assume(condTok, elseBranch.isModified());
}
}
Expand Down Expand Up @@ -742,19 +743,16 @@ struct ForwardTraversal {

};

Analyzer::Action valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings)
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings)
{
ForwardTraversal ft{a, settings};
ft.updateRange(start, end);
return ft.actions;
return {ft.actions, ft.terminate};
}

Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
{
ForwardTraversal ft{a, settings};
ft.updateRecursive(start);
return ft.actions;
return {ft.actions, ft.terminate};
}
10 changes: 5 additions & 5 deletions lib/forwardanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ class Settings;
class Token;
template <class T> class ValuePtr;

Analyzer::Action valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings);
Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings);

Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings);
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings);

#endif
Loading