Skip to content

Commit

Permalink
Fix 10314: Possible nullPointerRedundantCheck false positive (#3298)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pfultz2
pfultz2 authored Jun 19, 2021
1 parent 5922d51 commit dd178c3
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 122 deletions.
10 changes: 10 additions & 0 deletions lib/analyzer.h
View file
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
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
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
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

0 comments on commit dd178c3

Please sign in to comment.