From ef0443943f4348fe77bb1d473307cf73fee4ad90 Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 6 Jul 2026 11:56:28 -0500 Subject: [PATCH 1/4] Add inconclusive warning --- lib/forwardanalyzer.cpp | 12 +++++++++ lib/valueflow.cpp | 22 ++++++++++++++--- test/testnullpointer.cpp | 53 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 727c64b076d..c6b2ec4f2da 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -745,6 +745,13 @@ namespace { return Break(Analyzer::Terminate::Bail); if (updateScope(thenBranch.endBlock, depth - 1) == Progress::Break) return 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() && + !isEscapeScope(thenBranch.endBlock, thenBranch.escapeUnknown) && + thenBranch.escapeUnknown && !analyzer->lowerToInconclusive()) + return Break(Analyzer::Terminate::Bail); } else if (elseBranch.check) { // Likewise the skipped then block could still modify the value if (!condTok->hasKnownIntValue() && analyzeScope(thenBranch.endBlock).isModified() && @@ -752,6 +759,11 @@ namespace { return Break(Analyzer::Terminate::Bail); if (elseBranch.endBlock && updateScope(elseBranch.endBlock, depth - 1) == Progress::Break) return Break(); + // Same as above: an else branch that might not return + if (elseBranch.endBlock && !condTok->hasKnownIntValue() && + !isEscapeScope(elseBranch.endBlock, elseBranch.escapeUnknown) && + elseBranch.escapeUnknown && !analyzer->lowerToInconclusive()) + return Break(Analyzer::Terminate::Bail); } else { const bool conditional = stopOnCondition(condTok); // The value only flows into the then-branch when the condition can split diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index ef81fc772ac..8512c102bab 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4650,6 +4650,13 @@ struct ConditionHandler { }); } + static void lowerToInconclusive(std::list& values) { + for (ValueFlow::Value& v : values) { + if (!v.isImpossible()) + v.setInconclusive(); + } + } + void afterCondition(TokenList& tokenlist, const SymbolDatabase& symboldatabase, ErrorLogger& errorLogger, @@ -4882,10 +4889,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 {")) { @@ -4896,7 +4906,7 @@ struct ConditionHandler { if (!dead_else && unknownFunction) { if (settings.debugwarnings) bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); - return; + lowerToInconclusive(elseValues); } } @@ -4912,11 +4922,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()) diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 2483a967947..02810906fb2 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -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() { From 039a7c03083b8088419ee2c587fa97719482cf8e Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 6 Jul 2026 12:11:49 -0500 Subject: [PATCH 2/4] Add unit tests --- lib/valueflow.cpp | 3 ++- test/testvalueflow.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 8512c102bab..8c77b17cf94 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4650,7 +4650,8 @@ struct ConditionHandler { }); } - static void lowerToInconclusive(std::list& values) { + static void lowerToInconclusive(std::list& values) + { for (ValueFlow::Value& v : values) { if (!v.isImpossible()) v.setInconclusive(); diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 888710ce57d..14d1f9e8fa0 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -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() From 24290096d2ac83aa09075a6fb01cbe33d0f090e6 Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 6 Jul 2026 13:33:42 -0500 Subject: [PATCH 3/4] Refactor into reusable function --- lib/forwardanalyzer.cpp | 61 +++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index c6b2ec4f2da..16c41c485d0 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -407,6 +407,28 @@ namespace { 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() && + !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; @@ -558,16 +580,21 @@ namespace { 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) + { + if (!endBlock) + return Break(); + updateScopeState(endBlock); return updateRange(endBlock->link(), endBlock, depth); } @@ -738,32 +765,12 @@ namespace { 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(); - // 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() && - !isEscapeScope(thenBranch.endBlock, thenBranch.escapeUnknown) && - thenBranch.escapeUnknown && !analyzer->lowerToInconclusive()) - return Break(Analyzer::Terminate::Bail); } 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(); - // Same as above: an else branch that might not return - if (elseBranch.endBlock && !condTok->hasKnownIntValue() && - !isEscapeScope(elseBranch.endBlock, elseBranch.escapeUnknown) && - elseBranch.escapeUnknown && !analyzer->lowerToInconclusive()) - return Break(Analyzer::Terminate::Bail); } else { const bool conditional = stopOnCondition(condTok); // The value only flows into the then-branch when the condition can split From 0fdf7473d2a425690ebb00c2e0ac5d3248d589a7 Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 6 Jul 2026 13:36:04 -0500 Subject: [PATCH 4/4] Format --- lib/forwardanalyzer.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 16c41c485d0..6b05c2d0f42 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -423,8 +423,7 @@ namespace { // 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()) + if (!condTok->hasKnownIntValue() && !branch.escape && branch.escapeUnknown && !analyzer->lowerToInconclusive()) return Break(Analyzer::Terminate::Bail); return Progress::Continue; }