[clang] [analyzer] MallocChecker: Recognize std::atomics in smart pointer suppression. (PR #90918)

[Artem Dergachev via cfe-commits]

================
@@ -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