================
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
- if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
- // This immediately looks like a reference-counting destructor.
- // We're bad at guessing the original reference count of the
object,
- // so suppress the report for now.
- BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
- assert(!ReleaseDestructorLC &&
+ // See if we're releasing memory while inlining a destructor or
+ // functions that decrement reference counters (or one of its callees).
+ // This turns on various common false positive suppressions.
+ bool FoundAnyReleaseFunction = false;
+ for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
+ if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+ }
+ }
+
+ if (!FoundAnyReleaseFunction) {
----------------
NagyDonat wrote:
> And there is no warning. Did I get it wrong?
Your test code differs from the case that I tried to describe in two aspects:
1. The name of the class is recognized by
`isReferenceCountingPointerDestructor()` (which does case-insensitive substring
checks for several keywords, including "intrustive" and "ptr"), so the
use-after-free report is suppressed by that name-based heuristic, which
suppresses all use-after-free errors (directly or indirectly) within a
destructor of a class whose name is recognized.
2. Your `DecRef()` call is not recognized by the heuristics defined in
`MallocBugVisitor::VisitNode` [within the `if (ReleaseDestructorLC &&
(ReleaseDestructorLC == CurrentLC ||
ReleaseDestructorLC->isParentOf(CurrentLC)))` block] which searches for either
C11 atomic add/sub instructions or methods called on an object whose class name
contains "atomic".
If you rename the class (to avoid the other heuristic) and replace the
abstract/opaque `DecRef` calls with visibly atomic operations then you will get
a testcase that:
- is suppressed without your commit (because the visitor finds the destructor
which contains both the `delete` operation and the atomic call)
- is not suppressed with your commit (because the name-based heuristic doesn't
trigger AND the visitor only searches for the atomic call within the stack
frame that directly contains the `delete` operation, and that doesn't contain
an atomic call).
(Disclaimer: I didn't test this experimentally, I'm just reasoning based on the
source code.)
https://github.com/llvm/llvm-project/pull/104599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
