[clang] [analyzer] Remove redundant bug type DoubleDelete (PR #147542)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 9 07:26:41 PDT 2025
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/147542
>From 21d04521ec866b1d7850e2d7b758ceaa891359da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 8 Jul 2025 16:44:04 +0200
Subject: [PATCH 1/4] [analyzer] Remove redundant bug type DoubleDelete
This commit removes the DoubleDelete bug type from `MallocChecker.cpp`
because it's completely redundant with the `DoubleFree` bug (which is
already used for all allocator families, including new/delete).
This simplifies the code of the checker and prevents the potential
confusion caused by two semantically equivalent and very similar, but
not identical bug report messages.
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 33 +++----------------
.../test/Analysis/NewDelete-checker-test.cpp | 4 +--
2 files changed, 6 insertions(+), 31 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 0a58d7cc2635a..be8f89853bc96 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -346,10 +346,6 @@ namespace {
};
BUGTYPE_PROVIDER(DoubleFree, "Double free")
-// TODO: Remove DoubleDelete as a separate bug type and when it would be
-// emitted, emit DoubleFree reports instead. (Note that DoubleFree is already
-// used for all allocation families, not just malloc/free.)
-BUGTYPE_PROVIDER(DoubleDelete, "Double delete")
struct Leak : virtual public CheckerFrontend {
// Leaks should not be reported if they are post-dominated by a sink:
@@ -410,7 +406,7 @@ class MallocChecker
DynMemFrontend<DoubleFree, Leak, UseFree, BadFree, FreeAlloca, OffsetFree,
UseZeroAllocated>
MallocChecker;
- DynMemFrontend<DoubleFree, DoubleDelete, UseFree, BadFree, OffsetFree,
+ DynMemFrontend<DoubleFree, UseFree, BadFree, OffsetFree,
UseZeroAllocated>
NewDeleteChecker;
DynMemFrontend<Leak> NewDeleteLeaksChecker;
@@ -848,8 +844,6 @@ class MallocChecker
void HandleDoubleFree(CheckerContext &C, SourceRange Range, bool Released,
SymbolRef Sym, SymbolRef PrevSym) const;
- void HandleDoubleDelete(CheckerContext &C, SymbolRef Sym) const;
-
void HandleUseZeroAlloc(CheckerContext &C, SourceRange Range,
SymbolRef Sym) const;
@@ -2737,7 +2731,8 @@ void MallocChecker::HandleDoubleFree(CheckerContext &C, SourceRange Range,
(Released ? "Attempt to free released memory"
: "Attempt to free non-owned memory"),
N);
- R->addRange(Range);
+ if (Range.isValid())
+ R->addRange(Range);
R->markInteresting(Sym);
if (PrevSym)
R->markInteresting(PrevSym);
@@ -2746,26 +2741,6 @@ void MallocChecker::HandleDoubleFree(CheckerContext &C, SourceRange Range,
}
}
-void MallocChecker::HandleDoubleDelete(CheckerContext &C, SymbolRef Sym) const {
- const DoubleDelete *Frontend = getRelevantFrontendAs<DoubleDelete>(C, Sym);
- if (!Frontend)
- return;
- if (!Frontend->isEnabled()) {
- C.addSink();
- return;
- }
-
- if (ExplodedNode *N = C.generateErrorNode()) {
-
- auto R = std::make_unique<PathSensitiveBugReport>(
- Frontend->DoubleDeleteBug, "Attempt to delete released memory", N);
-
- R->markInteresting(Sym);
- R->addVisitor<MallocBugVisitor>(Sym);
- C.emitReport(std::move(R));
- }
-}
-
void MallocChecker::HandleUseZeroAlloc(CheckerContext &C, SourceRange Range,
SymbolRef Sym) const {
const UseZeroAllocated *Frontend =
@@ -3324,7 +3299,7 @@ void MallocChecker::checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
bool MallocChecker::checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const {
if (isReleased(Sym, C)) {
- HandleDoubleDelete(C, Sym);
+ HandleDoubleFree(C, SourceRange(), /*Released=*/true, Sym, nullptr);
return true;
}
return false;
diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp
index da0eef7c52bd8..7c3e142d586bb 100644
--- a/clang/test/Analysis/NewDelete-checker-test.cpp
+++ b/clang/test/Analysis/NewDelete-checker-test.cpp
@@ -412,7 +412,7 @@ class DerefClass{
void testDoubleDeleteClassInstance() {
DerefClass *foo = new DerefClass();
delete foo;
- delete foo; // newdelete-warning {{Attempt to delete released memory}}
+ delete foo; // newdelete-warning {{Attempt to free released memory}}
}
class EmptyClass{
@@ -424,7 +424,7 @@ class EmptyClass{
void testDoubleDeleteEmptyClass() {
EmptyClass *foo = new EmptyClass();
delete foo;
- delete foo; // newdelete-warning {{Attempt to delete released memory}}
+ delete foo; // newdelete-warning {{Attempt to free released memory}}
}
struct Base {
>From 81a9c315219218eb51cf0ec56f0ab427854b2f8f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 8 Jul 2025 17:20:49 +0200
Subject: [PATCH 2/4] Satisfy git-clang-format
---
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index be8f89853bc96..7252297c9fe9c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -406,8 +406,7 @@ class MallocChecker
DynMemFrontend<DoubleFree, Leak, UseFree, BadFree, FreeAlloca, OffsetFree,
UseZeroAllocated>
MallocChecker;
- DynMemFrontend<DoubleFree, UseFree, BadFree, OffsetFree,
- UseZeroAllocated>
+ DynMemFrontend<DoubleFree, UseFree, BadFree, OffsetFree, UseZeroAllocated>
NewDeleteChecker;
DynMemFrontend<Leak> NewDeleteLeaksChecker;
DynMemFrontend<FreeAlloca, MismatchedDealloc> MismatchedDeallocatorChecker;
>From ffbafe4aeabe794c198169a53c67bf2ab60057ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 9 Jul 2025 15:15:01 +0200
Subject: [PATCH 3/4] Use named param passing for PrevSym
---
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 7252297c9fe9c..f7271d34e48ec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3298,7 +3298,8 @@ void MallocChecker::checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
bool MallocChecker::checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const {
if (isReleased(Sym, C)) {
- HandleDoubleFree(C, SourceRange(), /*Released=*/true, Sym, nullptr);
+ HandleDoubleFree(C, SourceRange(), /*Released=*/true, Sym,
+ /*PrevSym=*/nullptr);
return true;
}
return false;
>From 15b8a18b0d0b706ac7a93c7b0591197cf9d27cff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 9 Jul 2025 16:21:06 +0200
Subject: [PATCH 4/4] Inline checkDoubleDelete, properly document the role of
this branch
I decided to inline and remove `checkDoubleDelete` because (1) it was
fairly short and (2) it is not a natural "do what is reasonable in
<a certain situation>" method so we cannot hide its implementation
behind a clear and descriptive name.
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 27 +++++++++----------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index f7271d34e48ec..30a04977d906d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -779,9 +779,6 @@ class MallocChecker
void checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
const Stmt *S) const;
- /// If in \p S \p Sym is being freed, check whether \p Sym was already freed.
- bool checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const;
-
/// Check if the function is known to free memory, or if it is
/// "interesting" and should be modeled explicitly.
///
@@ -3110,10 +3107,22 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
return;
}
+ // If we see a `CXXDestructorCall` (that is, an _implicit_ destructor call)
+ // to a region that's symbolic and known to be already freed, then it must be
+ // implicitly triggered by a `delete` expression. In this situation we should
+ // emit a `DoubleFree` report _now_ (before entering the call to the
+ // destructor) because otherwise the destructor call can trigger a
+ // use-after-free bug (by accessing any member variable) and that would be
+ // (technically valid, but) less user-friendly report than the `DoubleFree`.
if (const auto *DC = dyn_cast<CXXDestructorCall>(&Call)) {
SymbolRef Sym = DC->getCXXThisVal().getAsSymbol();
- if (!Sym || checkDoubleDelete(Sym, C))
+ if (!Sym)
+ return;
+ if (isReleased(Sym, C)) {
+ HandleDoubleFree(C, SourceRange(), /*Released=*/true, Sym,
+ /*PrevSym=*/nullptr);
return;
+ }
}
// We need to handle getline pre-conditions here before the pointed region
@@ -3295,16 +3304,6 @@ void MallocChecker::checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
}
}
-bool MallocChecker::checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const {
-
- if (isReleased(Sym, C)) {
- HandleDoubleFree(C, SourceRange(), /*Released=*/true, Sym,
- /*PrevSym=*/nullptr);
- return true;
- }
- return false;
-}
-
// Check if the location is a freed symbolic region.
void MallocChecker::checkLocation(SVal l, bool isLoad, const Stmt *S,
CheckerContext &C) const {
More information about the cfe-commits
mailing list