[clang] [analyzer] Fix false double free when including 3rd-party headers with overloaded delete operator as system headers (PR #85224)

Ella Ma via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 14 22:17:41 PDT 2024


================
@@ -1090,7 +1090,8 @@ static bool isStandardNewDelete(const FunctionDecl *FD) {
   // If the header for operator delete is not included, it's still defined
   // in an invalid source location. Check to make sure we don't crash.
   return !L.isValid() ||
-         FD->getASTContext().getSourceManager().isInSystemHeader(L);
+         (!FD->hasBody() && // FIXME: Still a false alarm after CTU inlining.
+          FD->getASTContext().getSourceManager().isInSystemHeader(L));
----------------
Snape3058 wrote:

> FD is not guaranteed to be the function decl which has the body

But `FunctionDecl::hasBody` will traverse all re-declarations and verify whether there is a decl has a body defined. So I think it will not affect the correctness no matter which re-declaration it points to.

My concern is the situation when the current TU does not have its definition but can be imported from other TUs during CTU inlining. Under this circumstance, the declaration can still pass the check here but trigger a false positive after CTU inlining.

```
+------------+   +------------------+   +----------------+
|checkPreCall|-->|No body in this TU|-->|Mark as Released|
+------------+   +------------------+   +----------------+
      |
      v
  +--------+   +---+   +----------------------+   +--------------------+
  |evalCall|-->|CTU|-->|Found body in other TU|-->|Inline external body|
  +--------+   +---+   +----------------------+   +--------------------+
      |
      v
+--------------------------------+   +-------------------------+
|the real delete in imported body|-->|double free (false alarm)|
+--------------------------------+   +-------------------------+
```

I am not sure whether there are other scenarios like this that can also trigger this false positive. If so, how can we update the conditions for these scenarios?

The current version is just a workaround for this bug we encountered here, rather than a robust verifier for checking whether a given overload is a standard one.

https://github.com/llvm/llvm-project/pull/85224


More information about the cfe-commits mailing list