diff --git a/.selfcheck_suppressions b/.selfcheck_suppressions index f21492e783a..402fdbeb75d 100644 --- 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 \ No newline at end of file diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 8dfd03596d3..f05f875d40a 100644 --- 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 index 7d3fa5c51ec..29322af4ac2 100644 --- 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 index 72b61fd68b9..e77641f89e7 100644 --- 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 index f499bb03b91..6e4c226e1d9 100644 --- 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 index 0999a08d908..670db0b1090 100644 --- 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 index 4afd2556a24..10f178abe50 100644 --- 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 index ba5c31d1f47..42fc855d5e9 100644 --- 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 index a8f675bbc97..7379957804f 100644 --- 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 index a50a3b1d4a4..10efc6d6f39 100644 --- 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 index 99ece1c57fc..369c04a2101 100644 --- 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"