From 1668b0b689885b703039af1eb27c76da77f6445d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Berder?= <18538310+francois-berder@users.noreply.github.com> Date: Tue, 25 Jun 2024 11:35:14 +0200 Subject: [PATCH] Fix #11311 Do not search for null pointer in dead code (#6442) --- Makefile | 2 +- lib/checknullpointer.cpp | 60 +++++++++++++++++++++++----------------- lib/checknullpointer.h | 2 +- lib/findtoken.h | 52 +++++++++++++++++++++++++++++----- oss-fuzz/Makefile | 2 +- test/testnullpointer.cpp | 42 ++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+), 36 deletions(-) diff --git a/Makefile b/Makefile index 56fd1b69ae3..d9e568f2788 100644 --- a/Makefile +++ b/Makefile @@ -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 diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 8c9cc1eb160..234d0c83e8f 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -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" @@ -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 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 { diff --git a/lib/checknullpointer.h b/lib/checknullpointer.h index f797a8f6d20..2b01c6b72a5 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -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(); diff --git a/lib/findtoken.h b/lib/findtoken.h index 73b4f0894f1..afb38a682f7 100644 --- a/lib/findtoken.h +++ b/lib/findtoken.h @@ -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)) { @@ -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; @@ -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; @@ -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; @@ -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); } @@ -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; } @@ -185,7 +192,8 @@ std::vector findTokensSkipDeadCode(const Library& library, result.push_back(tok); return false; }, - evaluate); + evaluate, + false); return result; } @@ -195,6 +203,35 @@ std::vector findTokensSkipDeadCode(const Library& library, T* start, const T return findTokensSkipDeadCode(library, start, end, pred, &evaluateKnownValues); } +template )> +std::vector findTokensSkipDeadAndUnevaluatedCode(const Library& library, + T* start, + const Token* end, + const Predicate& pred, + const Evaluate& evaluate) +{ + std::vector result; + (void)findTokensSkipDeadCodeImpl( + library, + start, + end, + pred, + [&](T* tok) { + result.push_back(tok); + return false; + }, + evaluate, + true); + return result; +} + +template )> +std::vector findTokensSkipDeadAndUnevaluatedCode(const Library& library, T* start, const Token* end, const Predicate& pred) +{ + return findTokensSkipDeadAndUnevaluatedCode(library, start, end, pred, &evaluateKnownValues); +} + + template )> T* findTokenSkipDeadCode(const Library& library, T* start, const Token* end, const Predicate& pred, const Evaluate& evaluate) { @@ -208,7 +245,8 @@ T* findTokenSkipDeadCode(const Library& library, T* start, const Token* end, con result = tok; return true; }, - evaluate); + evaluate, + false); return result; } diff --git a/oss-fuzz/Makefile b/oss-fuzz/Makefile index 9f3250394ba..96539e9f650 100644 --- a/oss-fuzz/Makefile +++ b/oss-fuzz/Makefile @@ -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 diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 432b50fab9c..a9a2a82043d 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -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); @@ -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"