[analyzer] Remove redundant bug type DoubleDelete #147542
Conversation
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThis commit removes the DoubleDelete bug type from 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. Full diff: https://github.com/llvm/llvm-project/pull/147542.diff 2 Files Affected:
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 {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -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}} | |||
There was a problem hiding this comment.
I'm not convinced that the message change is an improvement.
Can we somehow still use the delete spelling?
There was a problem hiding this comment.
One idea is to inspect the Expr against which we report the bug is a DeleteExpr or a callexpr.
There was a problem hiding this comment.
There are already many situations where cplusplus.NewDelete encounters repeated delete expressions and produces a DoubleFree bug report with "Attempt to free released memory" as the message, see e.g.
The DoubleDelete bug (which I want to remove / merge into DoubleFree) is only emitted in the special case when a nontrivial (?) destructor of the deleted type would be called, and it was introduced because in this case the body of the destructor is executed before the point where the DoubleFree bug is produced, so if the body of the destructor references a member variable, then (without the DoubleDelete code) we would get a use-after-free report instead of the DoubleFree (which is technically correct, but less elegant).
I think it's important that the warning message produced by delete foo; delete foo; should not depend on whether *foo has a destructor or not, so in this PR I would like to keep the spelling "Attempt to free released memory" (i.e. adjust the message produced by the special DoubleDelete case to match the more common DoubleFree message).
After this, I would gladly accept a separate followup PR that replaces "free" with a more accurate word, e.g. by
- changing the
DoubleFreemessage to "Attempt to release already released memory" (which would fit all allocator families); - or using "delete" in all the
DoubleFreereports that involvedeleteoperators (and not just those that come through theDoubleDeletelogic).
However, I don't have capacity for implementing this, as I'm already deep in the "also solve this tangentially related issue" mode.
There was a problem hiding this comment.
In my commit 15b8a18 I refactored this area of the code (by inlining checkDoubleDelete to remove a superfluous layer of indirection) and added a comment that describes the role of this "emit a DoubleDelete/DoubleFree when we see a destructor call to preempt the potential use-after-free report" branch.
There was a problem hiding this comment.
Given that this is the only user-visible change of this PR, I'm not happy to see this wording change, but I'm not interested in blocking this PR. I liked the Attempt to release already released memory phrasing that you suggested.
There was a problem hiding this comment.
I see your point and agree that in that particular case the spelling with free is slightly worse than the old message that said delete, but I think that overall the consistently slightly worse message is still better than the inconsistent situation where the checker could emit different messages in almost-identical situations.
I'll try to ensure that in the foreseeable future somebody (e.g. an intern in our team) systemically updates all the messages of the MallocChecker family to use properly generic vocabulary and e.g. say "release" instead of "free".
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.
|
The Windows CI failure is irrelevant:
|
This commit removes the DoubleDelete bug type from
MallocChecker.cppbecause it's completely redundant with theDoubleFreebug (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.