Skip to content

Commit b839fa5

Browse files
pfultz2Your Name
pfultz2
and
Your Name
authored
Fix issue 14892: Inconsistent nullPointerOutOfResources after possible noreturn function (#8700)
Co-authored-by: Your Name <you@example.com>
1 parent 41e87ac commit b839fa5

4 files changed

Lines changed: 143 additions & 19 deletions

File tree

‎lib/forwardanalyzer.cpp‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,27 @@ namespace {
412412
return p;
413413
}
414414

415+
// Update the branch that the evaluated condition takes
416+
Progress updateTakenBranch(Branch& branch, const Token* skippedBlock, const Token* condTok, int depth)
417+
{
418+
// The condition is only "known" because of an earlier assumption, so the
419+
// skipped block could still modify the value -> lower to possible
420+
if (!condTok->hasKnownIntValue() && skippedBlock && analyzeScope(skippedBlock).isModified() &&
421+
!analyzer->lowerToPossible())
422+
return Break(Analyzer::Terminate::Bail);
423+
if (!branch.endBlock)
424+
return Progress::Continue;
425+
updateScopeState(branch.endBlock);
426+
if (updateBranch(branch, depth - 1) == Progress::Break)
427+
return Progress::Break;
428+
// The branch was entered because of the tracked value; if it might not
429+
// return (it ends in a call to an unknown, possibly noreturn function)
430+
// then the value might not flow past the branch.
431+
if (!condTok->hasKnownIntValue() && !branch.escape && branch.escapeUnknown && !analyzer->lowerToInconclusive())
432+
return Break(Analyzer::Terminate::Bail);
433+
return Progress::Continue;
434+
}
435+
415436
bool reentersLoop(Token* endBlock, const Token* condTok, const Token* stepTok) const {
416437
if (!condTok)
417438
return true;
@@ -563,16 +584,21 @@ namespace {
563584
return updateLoop(endToken, endBlock, condTok, initTok, stepTok, true);
564585
}
565586

566-
Progress updateScope(Token* endBlock, int depth = 20)
587+
void updateScopeState(const Token* endBlock)
567588
{
568-
if (!endBlock)
569-
return Break();
570589
assert(endBlock->link());
571-
Token* ctx = endBlock->link()->previous();
590+
const Token* ctx = endBlock->link()->previous();
572591
if (Token::simpleMatch(ctx, ")"))
573592
ctx = ctx->link()->previous();
574593
if (ctx)
575594
analyzer->updateState(ctx);
595+
}
596+
597+
Progress updateScope(Token* endBlock, int depth = 20)
598+
{
599+
if (!endBlock)
600+
return Break();
601+
updateScopeState(endBlock);
576602
return updateRange(endBlock->link(), endBlock, depth);
577603
}
578604

@@ -743,19 +769,11 @@ namespace {
743769
const bool hasElse = Token::simpleMatch(endBlock, "} else {");
744770
tok = hasElse ? endBlock->linkAt(2) : endBlock;
745771
if (thenBranch.check) {
746-
// The condition is only "known" because of an earlier assumption, so the
747-
// skipped else block could still modify the value -> lower to possible
748-
if (!condTok->hasKnownIntValue() && hasElse &&
749-
analyzeScope(elseBranch.endBlock).isModified() && !analyzer->lowerToPossible())
750-
return Break(Analyzer::Terminate::Bail);
751-
if (updateScope(thenBranch.endBlock, depth - 1) == Progress::Break)
772+
if (updateTakenBranch(thenBranch, hasElse ? elseBranch.endBlock : nullptr, condTok, depth) ==
773+
Progress::Break)
752774
return Break();
753775
} else if (elseBranch.check) {
754-
// Likewise the skipped then block could still modify the value
755-
if (!condTok->hasKnownIntValue() && analyzeScope(thenBranch.endBlock).isModified() &&
756-
!analyzer->lowerToPossible())
757-
return Break(Analyzer::Terminate::Bail);
758-
if (elseBranch.endBlock && updateScope(elseBranch.endBlock, depth - 1) == Progress::Break)
776+
if (updateTakenBranch(elseBranch, thenBranch.endBlock, condTok, depth) == Progress::Break)
759777
return Break();
760778
} else {
761779
const bool conditional = stopOnCondition(condTok);

‎lib/valueflow.cpp‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4650,6 +4650,14 @@ struct ConditionHandler {
46504650
});
46514651
}
46524652

4653+
static void lowerToInconclusive(std::list<ValueFlow::Value>& values)
4654+
{
4655+
for (ValueFlow::Value& v : values) {
4656+
if (!v.isImpossible())
4657+
v.setInconclusive();
4658+
}
4659+
}
4660+
46534661
void afterCondition(TokenList& tokenlist,
46544662
const SymbolDatabase& symboldatabase,
46554663
ErrorLogger& errorLogger,
@@ -4882,10 +4890,13 @@ struct ConditionHandler {
48824890
else if (!dead_if)
48834891
dead_if = isReturnScope(after, settings.library, &unknownFunction);
48844892

4893+
// If the taken branch might not return (it ends in a call to an unknown,
4894+
// possibly noreturn function) then its values might not flow past the
4895+
// conditional code -> lower them to inconclusive.
48854896
if (!dead_if && unknownFunction) {
48864897
if (settings.debugwarnings)
48874898
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
4888-
return;
4899+
lowerToInconclusive(thenValues);
48894900
}
48904901

48914902
if (Token::simpleMatch(after, "} else {")) {
@@ -4896,7 +4907,7 @@ struct ConditionHandler {
48964907
if (!dead_else && unknownFunction) {
48974908
if (settings.debugwarnings)
48984909
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
4899-
return;
4910+
lowerToInconclusive(elseValues);
49004911
}
49014912
}
49024913

@@ -4912,11 +4923,15 @@ struct ConditionHandler {
49124923
std::copy_if(thenValues.cbegin(),
49134924
thenValues.cend(),
49144925
std::back_inserter(values),
4915-
std::mem_fn(&ValueFlow::Value::isPossible));
4926+
[](const ValueFlow::Value& v) {
4927+
return v.isPossible() || v.isInconclusive();
4928+
});
49164929
std::copy_if(elseValues.cbegin(),
49174930
elseValues.cend(),
49184931
std::back_inserter(values),
4919-
std::mem_fn(&ValueFlow::Value::isPossible));
4932+
[](const ValueFlow::Value& v) {
4933+
return v.isPossible() || v.isInconclusive();
4934+
});
49204935
}
49214936

49224937
if (values.empty())

‎test/testnullpointer.cpp‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4468,6 +4468,59 @@ class TestNullPointer : public TestFixture {
44684468
"[test.cpp:3:13]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n"
44694469
"[test.cpp:4:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
44704470
errout_str());
4471+
4472+
// the guard might call an unknown, possibly noreturn function -> no warning
4473+
check("void f() {\n"
4474+
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
4475+
" if (fid == NULL)\n"
4476+
" g();\n"
4477+
" fclose(fid);\n"
4478+
"}\n");
4479+
ASSERT_EQUALS("", errout_str());
4480+
4481+
// .. but an inconclusive warning is reported with --inconclusive
4482+
check("void f() {\n"
4483+
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
4484+
" if (fid == NULL)\n"
4485+
" g();\n"
4486+
" fclose(fid);\n"
4487+
"}\n",
4488+
dinit(CheckOptions, $.inconclusive = true));
4489+
ASSERT_EQUALS(
4490+
"[test.cpp:5:12]: (warning, inconclusive) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
4491+
errout_str());
4492+
4493+
check("int f(const int* p) {\n"
4494+
" if (p == nullptr)\n"
4495+
" g();\n"
4496+
" return *p;\n"
4497+
"}\n",
4498+
dinit(CheckOptions, $.inconclusive = true));
4499+
ASSERT_EQUALS(
4500+
"[test.cpp:2:11] -> [test.cpp:4:13]: (warning, inconclusive) Either the condition 'p==nullptr' is redundant or there is possible null pointer dereference: p. [nullPointerRedundantCheck]\n",
4501+
errout_str());
4502+
4503+
check("void f() {\n"
4504+
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
4505+
" if (fid != NULL)\n"
4506+
" ;\n"
4507+
" else\n"
4508+
" g();\n"
4509+
" fclose(fid);\n"
4510+
"}\n");
4511+
ASSERT_EQUALS("", errout_str());
4512+
4513+
// guard function is known to return -> warning
4514+
check("void g() {}\n"
4515+
"void f() {\n"
4516+
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
4517+
" if (fid == NULL)\n"
4518+
" g();\n"
4519+
" fclose(fid);\n"
4520+
"}\n");
4521+
ASSERT_EQUALS(
4522+
"[test.cpp:6:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
4523+
errout_str());
44714524
}
44724525

44734526
void functioncalllibrary() {

‎test/testvalueflow.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3866,6 +3866,44 @@ class TestValueFlow : public TestFixture {
38663866
" return x;\n"
38673867
"}\n";
38683868
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));
3869+
3870+
// if the guarded block calls an unknown, possibly noreturn function
3871+
// then the condition value is lowered to inconclusive after the block
3872+
code = "int f(int x) {\n"
3873+
" if (x == 0)\n"
3874+
" g();\n"
3875+
" return x;\n"
3876+
"}\n";
3877+
ASSERT_EQUALS(true, testValueOfXInconclusive(code, 4U, 0));
3878+
3879+
// .. also when the guard is in the else branch
3880+
code = "int f(int x) {\n"
3881+
" if (x != 0)\n"
3882+
" ;\n"
3883+
" else\n"
3884+
" g();\n"
3885+
" return x;\n"
3886+
"}\n";
3887+
ASSERT_EQUALS(true, testValueOfXInconclusive(code, 6U, 0));
3888+
3889+
// a declared function is assumed to return
3890+
code = "void g();\n"
3891+
"int f(int x) {\n"
3892+
" if (x == 0)\n"
3893+
" g();\n"
3894+
" return x;\n"
3895+
"}\n";
3896+
ASSERT_EQUALS(true, testValueOfX(code, 5U, 0));
3897+
ASSERT_EQUALS(false, testValueOfXInconclusive(code, 5U, 0));
3898+
3899+
// a noreturn function conclusively escapes
3900+
code = "int f(int x) {\n"
3901+
" if (x == 0)\n"
3902+
" abort();\n"
3903+
" return x;\n"
3904+
"}\n";
3905+
ASSERT_EQUALS(false, testValueOfX(code, 4U, 0));
3906+
ASSERT_EQUALS(true, testValueOfXImpossible(code, 4U, 0));
38693907
}
38703908

38713909
void valueFlowAfterConditionTernary()

0 commit comments

Comments
 (0)