Skip to content

Commit dd178c3

Browse files
pfultz2pfultz2
authored
Fix 10314: Possible nullPointerRedundantCheck false positive (cppcheck-opensource#3298)
1 parent 5922d51 commit dd178c3

6 files changed

Lines changed: 167 additions & 122 deletions

File tree

‎lib/analyzer.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,16 @@ struct Analyzer {
111111
unsigned int mFlag;
112112
};
113113

114+
enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };
115+
116+
struct Result {
117+
Result(Action action = Action::None, Terminate terminate = Terminate::None)
118+
: action(action), terminate(terminate)
119+
{}
120+
Action action;
121+
Terminate terminate;
122+
};
123+
114124
enum class Direction { Forward, Reverse };
115125

116126
struct Assume {

‎lib/astutils.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1563,7 +1563,8 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To
15631563
if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {"))
15641564
return isReturnScope(prev, library, unknownFunc, functionScope) &&
15651565
isReturnScope(prev->link()->tokAt(-2), library, unknownFunc, functionScope);
1566-
if (Token::simpleMatch(prev->link()->previous(), ") {") &&
1566+
// TODO: Check all cases
1567+
if (!functionScope && Token::simpleMatch(prev->link()->previous(), ") {") &&
15671568
Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") &&
15681569
!Token::findsimplematch(prev->link(), "break", prev)) {
15691570
return true;

‎lib/forwardanalyzer.cpp‎

Lines changed: 36 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,12 @@ struct ForwardTraversal {
2222
Analyzer::Action actions;
2323
bool analyzeOnly;
2424
bool analyzeTerminate;
25-
Terminate terminate = Terminate::None;
25+
Analyzer::Terminate terminate = Analyzer::Terminate::None;
2626
bool forked = false;
2727

28-
Progress Break(Terminate t = Terminate::None) {
29-
if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None)
28+
Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None)
29+
{
30+
if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
3031
terminate = t;
3132
return Progress::Break;
3233
}
@@ -89,7 +90,7 @@ struct ForwardTraversal {
8990
else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
9091
traverseRecursive(tok->astOperand1(), f, traverseUnknown);
9192
traverseRecursive(tok->astOperand2(), f, traverseUnknown);
92-
return Break(Terminate::Escape);
93+
return Break(Analyzer::Terminate::Escape);
9394
} else if (isUnevaluated(tok)) {
9495
if (out)
9596
*out = tok->link();
@@ -103,7 +104,7 @@ struct ForwardTraversal {
103104
// Skip lambdas
104105
} else if (T* lambdaEndToken = findLambdaEndToken(tok)) {
105106
if (checkScope(lambdaEndToken).isModified())
106-
return Break(Terminate::Bail);
107+
return Break(Analyzer::Terminate::Bail);
107108
if (out)
108109
*out = lambdaEndToken->next();
109110
// Skip class scope
@@ -152,7 +153,7 @@ struct ForwardTraversal {
152153
if (!checkThen && !checkElse) {
153154
// Stop if the value is conditional
154155
if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) {
155-
return Break(Terminate::Conditional);
156+
return Break(Analyzer::Terminate::Conditional);
156157
}
157158
checkThen = true;
158159
checkElse = true;
@@ -180,12 +181,12 @@ struct ForwardTraversal {
180181
if (!action.isNone() && !analyzeOnly)
181182
analyzer->update(tok, action, Analyzer::Direction::Forward);
182183
if (action.isInconclusive() && !analyzer->lowerToInconclusive())
183-
return Break(Terminate::Inconclusive);
184+
return Break(Analyzer::Terminate::Inconclusive);
184185
if (action.isInvalid())
185-
return Break(Terminate::Modified);
186+
return Break(Analyzer::Terminate::Modified);
186187
if (action.isWrite() && !action.isRead())
187188
// Analysis of this write will continue separately
188-
return Break(Terminate::Modified);
189+
return Break(Analyzer::Terminate::Modified);
189190
return Progress::Continue;
190191
}
191192

@@ -320,13 +321,13 @@ struct ForwardTraversal {
320321
if (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) {
321322
ForwardTraversal ft2 = fork(true);
322323
ft2.updateScope(branch.endBlock);
323-
if (ft2.terminate == Terminate::Escape) {
324+
if (ft2.terminate == Analyzer::Terminate::Escape) {
324325
branch.escape = true;
325326
branch.escapeUnknown = false;
326327
}
327328
}
328329
} else {
329-
if (ft1.front().terminate == Terminate::Escape) {
330+
if (ft1.front().terminate == Analyzer::Terminate::Escape) {
330331
branch.escape = true;
331332
branch.escapeUnknown = false;
332333
}
@@ -387,10 +388,10 @@ struct ForwardTraversal {
387388
actions |= allAnalysis;
388389
if (allAnalysis.isInconclusive()) {
389390
if (!analyzer->lowerToInconclusive())
390-
return Break(Terminate::Bail);
391+
return Break(Analyzer::Terminate::Bail);
391392
} else if (allAnalysis.isModified()) {
392393
if (!analyzer->lowerToPossible())
393-
return Break(Terminate::Bail);
394+
return Break(Analyzer::Terminate::Bail);
394395
}
395396
bool checkThen = true;
396397
bool checkElse = false;
@@ -409,9 +410,9 @@ struct ForwardTraversal {
409410
return Break();
410411
// If loop re-enters then it could be modified again
411412
if (allAnalysis.isModified() && reentersLoop(endBlock, condTok, stepTok))
412-
return Break(Terminate::Bail);
413+
return Break(Analyzer::Terminate::Bail);
413414
if (allAnalysis.isIncremental())
414-
return Break(Terminate::Bail);
415+
return Break(Analyzer::Terminate::Bail);
415416
} else {
416417
std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
417418
bool forkContinue = true;
@@ -425,9 +426,9 @@ struct ForwardTraversal {
425426
if (allAnalysis.isModified() || !forkContinue) {
426427
// TODO: Dont bail on missing condition
427428
if (!condTok)
428-
return Break(Terminate::Bail);
429+
return Break(Analyzer::Terminate::Bail);
429430
if (analyzer->isConditional() && stopUpdates())
430-
return Break(Terminate::Conditional);
431+
return Break(Analyzer::Terminate::Conditional);
431432
analyzer->assume(condTok, false);
432433
}
433434
if (forkContinue) {
@@ -437,7 +438,7 @@ struct ForwardTraversal {
437438
}
438439
}
439440
if (allAnalysis.isIncremental())
440-
return Break(Terminate::Bail);
441+
return Break(Analyzer::Terminate::Bail);
441442
}
442443
return Progress::Continue;
443444
}
@@ -451,7 +452,7 @@ struct ForwardTraversal {
451452
Progress updateRange(Token* start, const Token* end, int depth = 20) {
452453
forked = false;
453454
if (depth < 0)
454-
return Break(Terminate::Bail);
455+
return Break(Analyzer::Terminate::Bail);
455456
std::size_t i = 0;
456457
for (Token* tok = start; tok && tok != end; tok = tok->next()) {
457458
Token* next = nullptr;
@@ -485,13 +486,13 @@ struct ForwardTraversal {
485486
return Break();
486487
tok = skipTo(tok, scopeEndToken, end);
487488
if (!analyzer->lowerToPossible())
488-
return Break(Terminate::Bail);
489+
return Break(Analyzer::Terminate::Bail);
489490
// TODO: Don't break, instead move to the outer scope
490491
if (!tok)
491492
return Break();
492493
} else if (Token::Match(tok, "%name% :") || tok->str() == "case") {
493494
if (!analyzer->lowerToPossible())
494-
return Break(Terminate::Bail);
495+
return Break(Analyzer::Terminate::Bail);
495496
} else if (tok->link() && tok->str() == "}") {
496497
const Scope* scope = tok->scope();
497498
if (!scope)
@@ -505,7 +506,7 @@ struct ForwardTraversal {
505506
return Break();
506507
if (!condTok->hasKnownIntValue() || inLoop) {
507508
if (!analyzer->lowerToPossible())
508-
return Break(Terminate::Bail);
509+
return Break(Analyzer::Terminate::Bail);
509510
} else if (condTok->values().front().intvalue == inElse) {
510511
return Break();
511512
}
@@ -524,7 +525,7 @@ struct ForwardTraversal {
524525
tok = tok->linkAt(2);
525526
} else if (scope->type == Scope::eTry) {
526527
if (!analyzer->lowerToPossible())
527-
return Break(Terminate::Bail);
528+
return Break(Analyzer::Terminate::Bail);
528529
} else if (scope->type == Scope::eLambda) {
529530
return Break();
530531
} else if (scope->type == Scope::eDo && Token::simpleMatch(tok, "} while (")) {
@@ -595,32 +596,32 @@ struct ForwardTraversal {
595596
return Break();
596597
if (thenBranch.isDead() && elseBranch.isDead()) {
597598
if (thenBranch.isModified() && elseBranch.isModified())
598-
return Break(Terminate::Modified);
599+
return Break(Analyzer::Terminate::Modified);
599600
if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape())
600-
return Break(Terminate::Escape);
601-
return Break(Terminate::Bail);
601+
return Break(Analyzer::Terminate::Escape);
602+
return Break(Analyzer::Terminate::Bail);
602603
}
603604
// Conditional return
604605
if (thenBranch.isEscape() && !hasElse) {
605606
if (!thenBranch.isConclusiveEscape()) {
606607
if (!analyzer->lowerToInconclusive())
607-
return Break(Terminate::Bail);
608+
return Break(Analyzer::Terminate::Bail);
608609
} else if (thenBranch.check) {
609610
return Break();
610611
} else {
611612
if (analyzer->isConditional() && stopUpdates())
612-
return Break(Terminate::Conditional);
613+
return Break(Analyzer::Terminate::Conditional);
613614
analyzer->assume(condTok, false);
614615
}
615616
}
616617
if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) {
617618
if (!analyzer->lowerToInconclusive())
618-
return Break(Terminate::Bail);
619+
return Break(Analyzer::Terminate::Bail);
619620
} else if (thenBranch.isModified() || elseBranch.isModified()) {
620621
if (!hasElse && analyzer->isConditional() && stopUpdates())
621-
return Break(Terminate::Conditional);
622+
return Break(Analyzer::Terminate::Conditional);
622623
if (!analyzer->lowerToPossible())
623-
return Break(Terminate::Bail);
624+
return Break(Analyzer::Terminate::Bail);
624625
analyzer->assume(condTok, elseBranch.isModified());
625626
}
626627
}
@@ -742,19 +743,16 @@ struct ForwardTraversal {
742743

743744
};
744745

745-
Analyzer::Action valueFlowGenericForward(Token* start,
746-
const Token* end,
747-
const ValuePtr<Analyzer>& a,
748-
const Settings* settings)
746+
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings)
749747
{
750748
ForwardTraversal ft{a, settings};
751749
ft.updateRange(start, end);
752-
return ft.actions;
750+
return {ft.actions, ft.terminate};
753751
}
754752

755-
Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
753+
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
756754
{
757755
ForwardTraversal ft{a, settings};
758756
ft.updateRecursive(start);
759-
return ft.actions;
757+
return {ft.actions, ft.terminate};
760758
}

‎lib/forwardanalyzer.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@ class Settings;
2525
class Token;
2626
template <class T> class ValuePtr;
2727

28-
Analyzer::Action valueFlowGenericForward(Token* start,
29-
const Token* end,
30-
const ValuePtr<Analyzer>& a,
31-
const Settings* settings);
28+
Analyzer::Result valueFlowGenericForward(Token* start,
29+
const Token* end,
30+
const ValuePtr<Analyzer>& a,
31+
const Settings* settings);
3232

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

3535
#endif

0 commit comments

Comments
 (0)