Skip to content

Commit

Permalink
Fix #11311 Do not search for null pointer in dead code (#6442)
Browse files Browse the repository at this point in the history
  • Loading branch information
@francois-berder
francois-berder authored Jun 25, 2024
1 parent d19d8e8 commit 1668b0b
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 36 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ $(libcppdir)/checkleakautovar.o: lib/checkleakautovar.cpp lib/addoninfo.h lib/as
$(libcppdir)/checkmemoryleak.o: lib/checkmemoryleak.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkmemoryleak.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkmemoryleak.cpp

$(libcppdir)/checknullpointer.o: lib/checknullpointer.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checknullpointer.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
$(libcppdir)/checknullpointer.o: lib/checknullpointer.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checknullpointer.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/findtoken.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checknullpointer.cpp

$(libcppdir)/checkother.o: lib/checkother.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkother.h lib/config.h lib/errortypes.h lib/fwdanalysis.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
Expand Down
60 changes: 34 additions & 26 deletions lib/checknullpointer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "ctu.h"
#include "errorlogger.h"
#include "errortypes.h"
#include "findtoken.h"
#include "library.h"
#include "mathlib.h"
#include "settings.h"
Expand Down Expand Up @@ -279,47 +280,54 @@ static bool isNullablePointer(const Token* tok)
return false;
}

void CheckNullPointer::nullPointerByDeRefAndChec()
void CheckNullPointer::nullPointerByDeRefAndCheck()
{
const bool printInconclusive = (mSettings->certainty.isEnabled(Certainty::inconclusive));

for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
if (isUnevaluated(tok)) {
tok = tok->linkAt(1);
continue;
}
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope * scope : symbolDatabase->functionScopes) {
auto pred = [printInconclusive](const Token* tok) -> bool {
if (!tok)
return false;

if (Token::Match(tok, "%num%|%char%|%str%"))
continue;
if (Token::Match(tok, "%num%|%char%|%str%"))
return false;

if (!isNullablePointer(tok) ||
(tok->str() == "." && isNullablePointer(tok->astOperand2()) && tok->astOperand2()->getValue(0))) // avoid duplicate warning
continue;
if (!isNullablePointer(tok) ||
(tok->str() == "." && isNullablePointer(tok->astOperand2()) && tok->astOperand2()->getValue(0))) // avoid duplicate warning
return false;

// Can pointer be NULL?
const ValueFlow::Value *value = tok->getValue(0);
if (!value)
continue;
// Can pointer be NULL?
const ValueFlow::Value *value = tok->getValue(0);
if (!value)
return false;

if (!printInconclusive && value->isInconclusive())
continue;
if (!printInconclusive && value->isInconclusive())
return false;

// Pointer dereference.
bool unknown = false;
if (!isPointerDeRef(tok,unknown)) {
if (unknown)
nullPointerError(tok, tok->expressionString(), value, true);
continue;
}
return true;
};
std::vector<const Token *> tokens = findTokensSkipDeadAndUnevaluatedCode(mSettings->library, scope->bodyStart, scope->bodyEnd, pred);
for (const Token *tok : tokens) {
const ValueFlow::Value *value = tok->getValue(0);

// Pointer dereference.
bool unknown = false;
if (!isPointerDeRef(tok, unknown)) {
if (unknown)
nullPointerError(tok, tok->expressionString(), value, true);
continue;
}

nullPointerError(tok, tok->expressionString(), value, value->isInconclusive());
nullPointerError(tok, tok->expressionString(), value, value->isInconclusive());
}
}
}

void CheckNullPointer::nullPointer()
{
logChecker("CheckNullPointer::nullPointer");
nullPointerByDeRefAndChec();
nullPointerByDeRefAndCheck();
}

namespace {
Expand Down
2 changes: 1 addition & 1 deletion lib/checknullpointer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class CPPCHECKLIB CheckNullPointer : public Check {
* @brief Does one part of the check for nullPointer().
* Dereferencing a pointer and then checking if it's NULL..
*/
void nullPointerByDeRefAndChec();
void nullPointerByDeRefAndCheck();

/** undefined null pointer arithmetic */
void arithmetic();
Expand Down
52 changes: 45 additions & 7 deletions lib/findtoken.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
const Token* end,
const Predicate& pred,
Found found,
const Evaluate& evaluate)
const Evaluate& evaluate,
bool skipUnevaluated)
{
for (T* tok = start; precedes(tok, end); tok = tok->next()) {
if (pred(tok)) {
Expand All @@ -98,7 +99,7 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
auto result = evaluate(condTok);
if (result.empty())
continue;
if (findTokensSkipDeadCodeImpl(library, tok->next(), tok->linkAt(1), pred, found, evaluate))
if (findTokensSkipDeadCodeImpl(library, tok->next(), tok->linkAt(1), pred, found, evaluate, skipUnevaluated))
return true;
T* thenStart = tok->linkAt(1)->next();
T* elseStart = nullptr;
Expand All @@ -108,7 +109,7 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
int r = result.front();
if (r == 0) {
if (elseStart) {
if (findTokensSkipDeadCodeImpl(library, elseStart, elseStart->link(), pred, found, evaluate))
if (findTokensSkipDeadCodeImpl(library, elseStart, elseStart->link(), pred, found, evaluate, skipUnevaluated))
return true;
if (isReturnScope(elseStart->link(), library))
return true;
Expand All @@ -117,7 +118,7 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
tok = thenStart->link();
}
} else {
if (findTokensSkipDeadCodeImpl(library, thenStart, thenStart->link(), pred, found, evaluate))
if (findTokensSkipDeadCodeImpl(library, thenStart, thenStart->link(), pred, found, evaluate, skipUnevaluated))
return true;
if (isReturnScope(thenStart->link(), library))
return true;
Expand All @@ -137,7 +138,7 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
if (!cond) {
next = colon;
} else {
if (findTokensSkipDeadCodeImpl(library, tok->astParent()->next(), colon, pred, found, evaluate))
if (findTokensSkipDeadCodeImpl(library, tok->astParent()->next(), colon, pred, found, evaluate, skipUnevaluated))
return true;
next = nextAfterAstRightmostLeaf(colon);
}
Expand All @@ -164,6 +165,12 @@ bool findTokensSkipDeadCodeImpl(const Library& library,
else
tok = afterCapture;
}
if (skipUnevaluated && isUnevaluated(tok)) {
T *next = tok->linkAt(1);
if (!next)
continue;
tok = next;
}
}
return false;
}
Expand All @@ -185,7 +192,8 @@ std::vector<T*> findTokensSkipDeadCode(const Library& library,
result.push_back(tok);
return false;
},
evaluate);
evaluate,
false);
return result;
}

Expand All @@ -195,6 +203,35 @@ std::vector<T*> findTokensSkipDeadCode(const Library& library, T* start, const T
return findTokensSkipDeadCode(library, start, end, pred, &evaluateKnownValues);
}

template<class T, class Predicate, class Evaluate, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
std::vector<T*> findTokensSkipDeadAndUnevaluatedCode(const Library& library,
T* start,
const Token* end,
const Predicate& pred,
const Evaluate& evaluate)
{
std::vector<T*> result;
(void)findTokensSkipDeadCodeImpl(
library,
start,
end,
pred,
[&](T* tok) {
result.push_back(tok);
return false;
},
evaluate,
true);
return result;
}

template<class T, class Predicate, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
std::vector<T*> findTokensSkipDeadAndUnevaluatedCode(const Library& library, T* start, const Token* end, const Predicate& pred)
{
return findTokensSkipDeadAndUnevaluatedCode(library, start, end, pred, &evaluateKnownValues);
}


template<class T, class Predicate, class Evaluate, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
T* findTokenSkipDeadCode(const Library& library, T* start, const Token* end, const Predicate& pred, const Evaluate& evaluate)
{
Expand All @@ -208,7 +245,8 @@ T* findTokenSkipDeadCode(const Library& library, T* start, const Token* end, con
result = tok;
return true;
},
evaluate);
evaluate,
false);
return result;
}

Expand Down
2 changes: 1 addition & 1 deletion oss-fuzz/Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ $(libcppdir)/checkleakautovar.o: ../lib/checkleakautovar.cpp ../lib/addoninfo.h
$(libcppdir)/checkmemoryleak.o: ../lib/checkmemoryleak.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkmemoryleak.h ../lib/color.h ../lib/config.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkmemoryleak.cpp

$(libcppdir)/checknullpointer.o: ../lib/checknullpointer.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checknullpointer.h ../lib/color.h ../lib/config.h ../lib/ctu.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
$(libcppdir)/checknullpointer.o: ../lib/checknullpointer.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checknullpointer.h ../lib/color.h ../lib/config.h ../lib/ctu.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/findtoken.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checknullpointer.cpp

$(libcppdir)/checkother.o: ../lib/checkother.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkother.h ../lib/config.h ../lib/errortypes.h ../lib/fwdanalysis.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/suppressions.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
Expand Down
42 changes: 42 additions & 0 deletions test/testnullpointer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ class TestNullPointer : public TestFixture {
TEST_CASE(nullpointer_in_typeid);
TEST_CASE(nullpointer_in_alignof); // #11401
TEST_CASE(nullpointer_in_for_loop);
TEST_CASE(nullpointerDeadCode); // #11311
TEST_CASE(nullpointerDelete);
TEST_CASE(nullpointerSubFunction);
TEST_CASE(nullpointerExit);
Expand Down Expand Up @@ -3657,6 +3658,47 @@ class TestNullPointer : public TestFixture {
ASSERT_EQUALS("", errout_str());
}

void nullpointerDeadCode() {
// Ticket #11311
check ("void f() {\n"
" if (0)\n"
" *(int *)0 = 1;\n"
" else\n"
" ;\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check ("void f() {\n"
" if (0)\n"
" *(int *)0 = 1;\n"
" else {\n"
" if (0)\n"
" *(int *)0 = 2;\n"
" else\n"
" ;\n"
" }\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check ("void f() {\n"
" while(0)\n"
" *(int*)0 = 1;\n"
" do {\n"
" } while(0);\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check ("int f() {\n"
" return 0 ? *(int*)0 = 1 : 1;\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check ("int f() {\n"
" return 1 ? 1 : *(int*)0 = 1;\n"
"}\n");
ASSERT_EQUALS("", errout_str());
}

void nullpointerDelete() {
check("void f() {\n"
" K *k = getK();\n"
Expand Down

1 comment on commit 1668b0b

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francois-berder This introduced a false negative: https://trac.cppcheck.net/ticket/13220.

Please sign in to comment.