================
@@ -3479,13 +3479,24 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC) {
+ if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
+ ReleaseDestructorLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
+ // Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
- if (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC)) {
+ BR.markInvalid(getTag(), S);
+ }
+ } else if (const auto *CE = dyn_cast<CallExpr>(S)) {
+ // Check for `std::atomic` and such. This covers both regular method
calls
+ // and operator calls.
+ if (const auto *MD =
+ dyn_cast_or_null<CXXMethodDecl>(CE->getDirectCallee())) {
+ const CXXRecordDecl *RD = MD->getParent();
+ // A bit wobbly with ".contains()" because it may be like
+ // "__atomic_base" or something.
+ if (StringRef(RD->getNameAsString()).contains("atomic")) {
----------------
haoNoQ wrote:
> Do we have any safeguard to only match names within the `std` namespace?
> Could you add a test case demonstrating that a user-defined type wouldn't be
> mistaken for `atomic` here?
There aren't any safeguards, but I'm not sure we want them. This is already a
crude heuristic that goes at like 45 degrees against the desired direction. I
think I'd rather have it catch more false positives by respecting user-defined
types that are probably atomics, than eliminate a few false negatives when our
tool is applied to... Chemistry software probably? A few video games come to
mind? Which are both amazing and I'd love to catch a few bugs in them. But I'm
generally more worried about the entire projects that can't use our tool at all
because they use custom atomic classes, dealing with problems similar to the
original bug report.
Because this heuristic applies only to method calls inside destructors (which
doesn't include other destructor calls), the exact situation where this causes
problems is _"somebody explicitly calls a method on a class named
'...atomic...' which isn't an actual atomic integer, in a destructor which
isn't a destructor of a smart pointer, and we're tracking a MallocChecker
use-after-free report where memory was released inside that destructor, and
that report is actually desired by the user"_. Which is definitely not
impossible, but even in projects where this could happen, it would not happen
every time; I hope that only one or two reports are affected in practice.
MallocChecker isn't even that good in C++ code in which the programmers know
what a destructor is, so I think even the "report is actually desired by the
user" part would be fairly hard to satisfy.
So I'm future-proofing this a bit, acknowledging that if we went with strict
`std::atomic` requirement, we'd be likely to relax it in the future when more
bug reports come in.
Dunno, am I being overly pessimistic? Again, I'm very open to changing my mind.
https://github.com/llvm/llvm-project/pull/90918
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
