Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 0 additions & 4 deletions lib/astutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
11 changes: 8 additions & 3 deletions lib/checkclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/checkcondition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion lib/checkstl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion lib/forwardanalyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
3 changes: 0 additions & 3 deletions lib/reverseanalyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 2 additions & 4 deletions lib/symboldatabase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/tokenize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 26 additions & 4 deletions lib/vf_analyzers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
134 changes: 134 additions & 0 deletions test/testnullpointer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
Loading