Fix issue 13682: false negative: nullPointerRedundantCheck (multiple conditions, regression) (#8669)

pfultz2 · Your Name · web-flow · commit 887b5d2ff530 · 2026-06-25T08:18:17.000+02:00
Co-authored-by: Your Name &lt;you@example.com&gt;
diff --git a/.selfcheck_suppressions b/.selfcheck_suppressions
@@ -79,3 +79,4 @@ useStlAlgorithm:externals/simplecpp/simplecpp.cpp
 funcArgNamesDifferentUnnamed:externals/simplecpp/simplecpp.h
 missingMemberCopy:externals/simplecpp/simplecpp.h
 shadowFunction:externals/simplecpp/simplecpp.h
+knownConditionTrueFalse:externals/simplecpp/simplecpp.cpp
diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -495,8 +495,6 @@ bool isTemporary(const Token* tok, const Library* library, bool unknown)
         }
         return unknown;
     }
-    if (tok->isCast())
-        return false;
     // Currying a function is unknown in cppcheck
     if (Token::simpleMatch(tok, "(") && Token::simpleMatch(tok->astOperand1(), "("))
         return unknown;
@@ -1543,8 +1541,6 @@ bool isUsedAsBool(const Token* const tok, const Settings& settings)
         return true;
     if (parent->isCast())
         return !Token::simpleMatch(parent->astOperand1(), "dynamic_cast") && isUsedAsBool(parent, settings);
-    if (parent->isUnaryOp("*"))
-        return isUsedAsBool(parent, settings);
     if (Token::Match(parent, "==|!=") && tok->valueType() && tok->valueType()->pointer &&
         tok->astSibling()->hasKnownIntValue() && tok->astSibling()->getKnownIntValue() == 0)
         return true;
diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp
@@ -3452,9 +3452,14 @@ void CheckClassImpl::checkUselessOverride()
 
                     if (isSameCode) {
                         // bailout for shadowed members
-                        if (!classScope->definedType ||
-                            !getDuplInheritedMembersRecursive(classScope->definedType, classScope->definedType, /*skipPrivate*/ false).empty() ||
-                            !getDuplInheritedMemberFunctionsRecursive(classScope->definedType, classScope->definedType, /*skipPrivate*/ false).empty())
+                        if (!getDuplInheritedMembersRecursive(classScope->definedType,
+                                                              classScope->definedType,
+                                                              /*skipPrivate*/ false)
+                            .empty() ||
+                            !getDuplInheritedMemberFunctionsRecursive(classScope->definedType,
+                                                                      classScope->definedType,
+                                                                      /*skipPrivate*/ false)
+                            .empty())
                             continue;
                         uselessOverrideError(baseFunc, &func, true);
                         continue;
diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
@@ -455,7 +455,7 @@ bool CheckConditionImpl::isOverlappingCond(const Token * const cond1, const Toke
         if (!num1->isNumber() || MathLib::isNegative(num1->str()))
             return false;
 
-        if (!Token::Match(cond2, "&|==") || !cond2->astOperand1() || !cond2->astOperand2())
+        if (!Token::Match(cond2, "&|==") || !cond2->astOperand1())
             return false;
         const Token *expr2 = cond2->astOperand1();
         const Token *num2  = cond2->astOperand2();
diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp
@@ -549,7 +549,7 @@ void CheckStlImpl::iterators()
                     }
 
                     // Not different containers if a reference is used..
-                    if (containerToken && containerToken->variable() && containerToken->variable()->isReference()) {
+                    if (containerToken->variable() && containerToken->variable()->isReference()) {
                         const Token *nameToken = containerToken->variable()->nameToken();
                         if (Token::Match(nameToken, "%name% =")) {
                             const Token *name1 = nameToken->tokAt(2);
diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp
@@ -782,7 +782,7 @@ namespace {
                             } else if (thenBranch.check) {
                                 return Break();
                             } else {
-                                if (analyzer->isConditional() && stopUpdates())
+                                if (stopOnCondition(condTok) && stopUpdates())
                                     return Break(Analyzer::Terminate::Conditional);
                                 analyzer->assume(condTok, false);
                             }
diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp
@@ -340,9 +340,6 @@ namespace {
                         valueFlowGenericForward(condTok, analyzer, tokenlist, errorLogger, settings);
                     else if (condAction.isRead())
                         break;
-                    // If the condition modifies the variable then bail
-                    if (condAction.isModified())
-                        break;
                     tok = jumpToStart(tok->link());
                     continue;
                 }
diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp
@@ -1043,10 +1043,8 @@ void SymbolDatabase::createSymbolDatabaseNeedInitialization()
                             scope.definedType->needInitialization = Type::NeedInitialization::True;
                         else if (!unknown)
                             scope.definedType->needInitialization = Type::NeedInitialization::False;
-                        else {
-                            if (scope.definedType->needInitialization == Type::NeedInitialization::Unknown)
-                                unknowns++;
-                        }
+                        else
+                            unknowns++;
                     }
                 } else if (scope.type == ScopeType::eUnion && scope.definedType->needInitialization == Type::NeedInitialization::Unknown)
                     scope.definedType->needInitialization = Type::NeedInitialization::True;
diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp
@@ -2711,7 +2711,7 @@ namespace {
     {
         Token *tok1 = tok;
 
-        if (tok1 && tok1->str() != nameToken->str())
+        if (!tok1 || tok1->str() != nameToken->str())
             return false;
 
         // skip this using
diff --git a/lib/vf_analyzers.cpp b/lib/vf_analyzers.cpp
@@ -1198,18 +1198,40 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer {
 
     bool stopOnCondition(const Token* condTok) const override
     {
-        if (value.isNonValue())
-            return false;
         if (value.isImpossible())
             return false;
-        if (isConditional() && !value.isKnown())
+        // lifetime values must keep flowing to properly track aliases
+        if (value.isLifetimeValue())
+            return false;
+        // 'conditional' flag (uninit, or lowered after a modifying branch): may depend on a
+        // condition that doesn't mention the variable -> stop
+        if (value.conditional && !value.isKnown())
             return true;
-        if (value.isSymbolicValue())
+        if (value.isNonValue())
             return false;
+        if (value.isSymbolicValue())
+            return isConditional() && !value.isKnown();
+        // conditional via the originating 'condition' (e.g. possible null after 'if (p && ...)'): only flow
+        // if the condition references the value, else a correlation we can't follow (e.g.
+        // 'bool ok = (p != nullptr); if (!ok)') could make a later deref safe -> stop
+        if (value.condition && !value.isKnown() && !conditionReferencesValue(condTok))
+            return true;
         ConditionState cs = analyzeCondition(condTok);
         return cs.isUnknownDependent();
     }
 
+    // Does the condition mention the tracked value, either directly or through a symbolic alias?
+    bool conditionReferencesValue(const Token* condTok) const
+    {
+        return findAstNode(condTok, [&](const Token* tok) {
+            if (match(tok))
+                return true;
+            return std::any_of(tok->values().cbegin(), tok->values().cend(), [&](const ValueFlow::Value& v) {
+                return v.isSymbolicValue() && !v.isImpossible() && v.tokvalue && match(v.tokvalue);
+            });
+        }) != nullptr;
+    }
+
     bool updateScope(const Token* endBlock, bool /*modified*/) const override {
         const Scope* scope = endBlock->scope();
         if (!scope)
diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp
@@ -143,6 +143,8 @@ class TestNullPointer : public TestFixture {
         TEST_CASE(nullpointer103);
         TEST_CASE(nullpointer104); // #13881
         TEST_CASE(nullpointer105); // #13861
+        TEST_CASE(nullpointer106); // #13682
+        TEST_CASE(nullpointer107); // #13682 (FP/FN cases around guards that depend on the pointer indirectly)
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2966,6 +2968,138 @@ class TestNullPointer : public TestFixture {
         ASSERT_EQUALS("", errout_str());
     }
 
+    void nullpointer106() // #13682
+    {
+        // An unrelated condition between the null check and the dereference must not stop the analysis
+        check("struct S {\n"
+              "    bool b;\n"
+              "    bool f() const;\n"
+              "};\n"
+              "void f(const S* p, const S* o) {\n"
+              "    const S* p1 = p;\n"
+              "    if (p1 && p1->f())\n"
+              "        return;\n"
+              "    if (p == o)\n"
+              "        return;\n"
+              "    if (p1->b) {}\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:7:9] -> [test.cpp:11:9]: (warning) Either the condition 'p1' is redundant or there is possible null pointer dereference: p1. [nullPointerRedundantCheck]\n",
+            errout_str());
+    }
+
+    void nullpointer107() // #13682 - guards that depend on the pointer indirectly
+    {
+        // cached null-check 'ok'; guard 'if (!ok)' is safe -> no FP
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p) {\n"
+              "    bool ok = (p != nullptr);\n"
+              "    if (p && p->f())\n"
+              "        return;\n"
+              "    if (!ok)\n"
+              "        return;\n"
+              "    p->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        // unrelated bool guard -> conservative, no FP
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p, bool valid) {\n"
+              "    S* p1 = p;\n"
+              "    if (p1 && p1->f())\n"
+              "        return;\n"
+              "    if (!valid)\n"
+              "        return;\n"
+              "    p1->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        // guard on a different pointer -> no FP
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p, S* q) {\n"
+              "    S* p1 = p;\n"
+              "    if (p1 && p1->f())\n"
+              "        return;\n"
+              "    if (!q)\n"
+              "        return;\n"
+              "    p1->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        // direct null guard on the alias -> no FP
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p) {\n"
+              "    S* p1 = p;\n"
+              "    if (p1 && p1->f())\n"
+              "        return;\n"
+              "    if (!p)\n"
+              "        return;\n"
+              "    p1->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        // FN: 'if (ok)' => survivor has p==nullptr, but the cached 'ok' is not followed -> should warn
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p) {\n"
+              "    bool ok = (p != nullptr);\n"
+              "    if (p && p->f())\n"
+              "        return;\n"
+              "    if (ok)\n"
+              "        return;\n"
+              "    p->g();\n"
+              "}\n");
+        TODO_ASSERT_EQUALS(
+            "[test.cpp:4:9] -> [test.cpp:8:5]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p. [nullPointerRedundantCheck]\n",
+            "",
+            errout_str());
+
+        // FN: sink(q) drops the q==p symbolic, so guard 'if (q)' is no longer seen to relate to p -> should warn
+        check("struct S { void g(); bool f() const; };\n"
+              "void sink(S*&);\n"
+              "void f(S* p) {\n"
+              "    S* q = p;\n"
+              "    if (p && p->f())\n"
+              "        return;\n"
+              "    sink(q);\n"
+              "    if (q)\n"
+              "        return;\n"
+              "    p->g();\n"
+              "}\n");
+        TODO_ASSERT_EQUALS(
+            "[test.cpp:5:9] -> [test.cpp:10:5]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p. [nullPointerRedundantCheck]\n",
+            "",
+            errout_str());
+
+        // a conditional modification makes ProgramMemory drop the guard (FP-prone) -> must stay quiet:
+        // alias 'q==p' re-assigned to p under a condition
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p, bool c) {\n"
+              "    S* q = p;\n"
+              "    if (p && p->f())\n"
+              "        return;\n"
+              "    if (c)\n"
+              "        q = p;\n"
+              "    if (!q)\n"
+              "        return;\n"
+              "    p->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        // cached 'ok' refreshed under a condition
+        check("struct S { void g(); bool f() const; };\n"
+              "void f(S* p, bool c) {\n"
+              "    bool ok = (p != nullptr);\n"
+              "    if (p && p->f())\n"
+              "        return;\n"
+              "    if (c)\n"
+              "        ok = (p != nullptr);\n"
+              "    if (!ok)\n"
+              "        return;\n"
+              "    p->g();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"

Original file line number	Diff line number	Diff line change
`@@ -549,7 +549,7 @@ void CheckStlImpl::iterators()`
`549`	`549`	`}`
`550`	`550`
`551`	`551`	`// Not different containers if a reference is used..`
`552`		`- if (containerToken && containerToken->variable() && containerToken->variable()->isReference()) {`
	`552`	`+ if (containerToken->variable() && containerToken->variable()->isReference()) {`
`553`	`553`	`const Token *nameToken = containerToken->variable()->nameToken();`
`554`	`554`	`if (Token::Match(nameToken, "%name% =")) {`
`555`	`555`	`const Token *name1 = nameToken->tokAt(2);`
Original file line number	Diff line number	Diff line change
`@@ -782,7 +782,7 @@ namespace {`
`782`	`782`	`} else if (thenBranch.check) {`
`783`	`783`	`return Break();`
`784`	`784`	`} else {`
`785`		`- if (analyzer->isConditional() && stopUpdates())`
	`785`	`+ if (stopOnCondition(condTok) && stopUpdates())`
`786`	`786`	`return Break(Analyzer::Terminate::Conditional);`
`787`	`787`	`analyzer->assume(condTok, false);`
`788`	`788`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2711,7 +2711,7 @@ namespace {`
`2711`	`2711`	`{`
`2712`	`2712`	`Token *tok1 = tok;`
`2713`	`2713`
`2714`		`- if (tok1 && tok1->str() != nameToken->str())`
	`2714`	`+ if (!tok1 \|\| tok1->str() != nameToken->str())`
`2715`	`2715`	`return false;`
`2716`	`2716`
`2717`	`2717`	`// skip this using`