Skip to content

Fix issue 14892: Inconsistent nullPointerOutOfResources after possible noreturn function #8700

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

Open
pfultz2 wants to merge 4 commits into
base: main
Choose a base branch
from
+143 −19
Open

Fix issue 14892: Inconsistent nullPointerOutOfResources after possible noreturn function #8700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef04439
Add inconclusive warning
Jul 6, 2026
039a7c0
Add unit tests
Jul 6, 2026
2429009
Refactor into reusable function
Jul 6, 2026
0fdf747
Format
Jul 6, 2026
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
48 changes: 33 additions & 15 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,27 @@
return p;
}

// Update the branch that the evaluated condition takes
Progress updateTakenBranch(Branch& branch, const Token* skippedBlock, const Token* condTok, int depth)
{
// The condition is only "known" because of an earlier assumption, so the
// skipped block could still modify the value -> lower to possible
if (!condTok->hasKnownIntValue() && skippedBlock && analyzeScope(skippedBlock).isModified() &&

Check warning

Code scanning / Cppcheck Premium

Do not dereference null pointers Warning

Do not dereference null pointers
!analyzer->lowerToPossible())
return Break(Analyzer::Terminate::Bail);
if (!branch.endBlock)
return Progress::Continue;
updateScopeState(branch.endBlock);
if (updateBranch(branch, depth - 1) == Progress::Break)
return Progress::Break;
// The branch was entered because of the tracked value; if it might not
// return (it ends in a call to an unknown, possibly noreturn function)
// then the value might not flow past the branch.
if (!condTok->hasKnownIntValue() && !branch.escape && branch.escapeUnknown && !analyzer->lowerToInconclusive())
return Break(Analyzer::Terminate::Bail);
return Progress::Continue;
}

bool reentersLoop(Token* endBlock, const Token* condTok, const Token* stepTok) const {
if (!condTok)
return true;
Expand Down Expand Up @@ -558,16 +579,21 @@
return updateLoop(endToken, endBlock, condTok, initTok, stepTok, true);
}

Progress updateScope(Token* endBlock, int depth = 20)
void updateScopeState(const Token* endBlock)
{
if (!endBlock)
return Break();
assert(endBlock->link());
Token* ctx = endBlock->link()->previous();
const Token* ctx = endBlock->link()->previous();
if (Token::simpleMatch(ctx, ")"))
ctx = ctx->link()->previous();
if (ctx)
analyzer->updateState(ctx);
}

Progress updateScope(Token* endBlock, int depth = 20)

Check warning

Code scanning / Cppcheck Premium

Use meaningful symbolic constants to represent literal values. Warning

Use meaningful symbolic constants to represent literal values.
{
if (!endBlock)
return Break();
updateScopeState(endBlock);
return updateRange(endBlock->link(), endBlock, depth);
}

Expand Down Expand Up @@ -738,19 +764,11 @@
const bool hasElse = Token::simpleMatch(endBlock, "} else {");
tok = hasElse ? endBlock->linkAt(2) : endBlock;
if (thenBranch.check) {
// The condition is only "known" because of an earlier assumption, so the
// skipped else block could still modify the value -> lower to possible
if (!condTok->hasKnownIntValue() && hasElse &&
analyzeScope(elseBranch.endBlock).isModified() && !analyzer->lowerToPossible())
return Break(Analyzer::Terminate::Bail);
if (updateScope(thenBranch.endBlock, depth - 1) == Progress::Break)
if (updateTakenBranch(thenBranch, hasElse ? elseBranch.endBlock : nullptr, condTok, depth) ==
Progress::Break)
return Break();
} else if (elseBranch.check) {
// Likewise the skipped then block could still modify the value
if (!condTok->hasKnownIntValue() && analyzeScope(thenBranch.endBlock).isModified() &&
!analyzer->lowerToPossible())
return Break(Analyzer::Terminate::Bail);
if (elseBranch.endBlock && updateScope(elseBranch.endBlock, depth - 1) == Progress::Break)
if (updateTakenBranch(elseBranch, thenBranch.endBlock, condTok, depth) == Progress::Break)
return Break();
} else {
const bool conditional = stopOnCondition(condTok);
Expand Down
23 changes: 19 additions & 4 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4650,6 +4650,14 @@ struct ConditionHandler {
});
}

static void lowerToInconclusive(std::list<ValueFlow::Value>& values)
{
for (ValueFlow::Value& v : values) {
if (!v.isImpossible())
v.setInconclusive();
}
}

void afterCondition(TokenList& tokenlist,
const SymbolDatabase& symboldatabase,
ErrorLogger& errorLogger,
Expand Down Expand Up @@ -4882,10 +4890,13 @@ struct ConditionHandler {
else if (!dead_if)
dead_if = isReturnScope(after, settings.library, &unknownFunction);

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

if (Token::simpleMatch(after, "} else {")) {
Expand All @@ -4896,7 +4907,7 @@ struct ConditionHandler {
if (!dead_else && unknownFunction) {
if (settings.debugwarnings)
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
return;
lowerToInconclusive(elseValues);
}
}

Expand All @@ -4912,11 +4923,15 @@ struct ConditionHandler {
std::copy_if(thenValues.cbegin(),
thenValues.cend(),
std::back_inserter(values),
std::mem_fn(&ValueFlow::Value::isPossible));
[](const ValueFlow::Value& v) {
return v.isPossible() || v.isInconclusive();
});
std::copy_if(elseValues.cbegin(),
elseValues.cend(),
std::back_inserter(values),
std::mem_fn(&ValueFlow::Value::isPossible));
[](const ValueFlow::Value& v) {
return v.isPossible() || v.isInconclusive();
});
}

if (values.empty())
Expand Down
53 changes: 53 additions & 0 deletions test/testnullpointer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4468,6 +4468,59 @@ class TestNullPointer : public TestFixture {
"[test.cpp:3:13]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n"
"[test.cpp:4:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
errout_str());

// the guard might call an unknown, possibly noreturn function -> no warning
check("void f() {\n"
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
" if (fid == NULL)\n"
" g();\n"
" fclose(fid);\n"
"}\n");
ASSERT_EQUALS("", errout_str());

// .. but an inconclusive warning is reported with --inconclusive
check("void f() {\n"
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
" if (fid == NULL)\n"
" g();\n"
" fclose(fid);\n"
"}\n",
dinit(CheckOptions, $.inconclusive = true));
ASSERT_EQUALS(
"[test.cpp:5:12]: (warning, inconclusive) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
errout_str());

check("int f(const int* p) {\n"
" if (p == nullptr)\n"
" g();\n"
" return *p;\n"
"}\n",
dinit(CheckOptions, $.inconclusive = true));
ASSERT_EQUALS(
"[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",
errout_str());

check("void f() {\n"
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
" if (fid != NULL)\n"
" ;\n"
" else\n"
" g();\n"
" fclose(fid);\n"
"}\n");
ASSERT_EQUALS("", errout_str());

// guard function is known to return -> warning
check("void g() {}\n"
"void f() {\n"
" FILE* fid = fopen(\"x.txt\", \"w\");\n"
" if (fid == NULL)\n"
" g();\n"
" fclose(fid);\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:6:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n",
errout_str());
}

void functioncalllibrary() {
Expand Down
38 changes: 38 additions & 0 deletions test/testvalueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3834,6 +3834,44 @@ class TestValueFlow : public TestFixture {
" return x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));

// if the guarded block calls an unknown, possibly noreturn function
// then the condition value is lowered to inconclusive after the block
code = "int f(int x) {\n"
" if (x == 0)\n"
" g();\n"
" return x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfXInconclusive(code, 4U, 0));

// .. also when the guard is in the else branch
code = "int f(int x) {\n"
" if (x != 0)\n"
" ;\n"
" else\n"
" g();\n"
" return x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfXInconclusive(code, 6U, 0));

// a declared function is assumed to return
code = "void g();\n"
"int f(int x) {\n"
" if (x == 0)\n"
" g();\n"
" return x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfX(code, 5U, 0));
ASSERT_EQUALS(false, testValueOfXInconclusive(code, 5U, 0));

// a noreturn function conclusively escapes
code = "int f(int x) {\n"
" if (x == 0)\n"
" abort();\n"
" return x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfX(code, 4U, 0));
ASSERT_EQUALS(true, testValueOfXImpossible(code, 4U, 0));
}

void valueFlowAfterConditionTernary()
Expand Down
Loading