[analyzer] Fix false double free when including 3rd-party headers with overloaded delete operator as system headers

[Snape3058]

Fixes #62985
Fixes #58820

When 3rd-party header files are included as system headers, their overloaded new and delete operators are also considered as the std ones. However, those overloaded operator functions will also be inlined. This makes the same
symbolic memory marked as released twice: during checkPreCall of the overloaded delete operator and when calling ::operator delete after inlining the overloaded operator function (if it has).

This patch attempts to fix this bug by adjusting the strategy of verifying whether the callee is a standard new or delete operator in the isStandardNewDelete function.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ella Ma (Snape3058)

Changes

Fixes #62985

When 3rd-party header files are included as system headers, their overloaded new and delete operators are also considered as the std ones. However, those overloaded operator functions will also be inlined. This makes the same
symbolic memory marked as released twice: during checkPreCall of the overloaded delete operator and when calling ::operator delete after inlining the overloaded operator function (if it has).

This patch attempts to fix this bug by adjusting the strategy of verifying whether the callee is a standard new or delete operator in the isStandardNewDelete function.

---

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

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+2-1)
• (added) clang/test/Analysis/Inputs/overloaded-delete-in-header.h (+17)
• (added) clang/test/Analysis/overloaded-delete-in-system-header.cpp (+25)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 03cb7696707fe2..c7ec2b7cc43b30 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -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));
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/Inputs/overloaded-delete-in-header.h b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h
new file mode 100644
index 00000000000000..5090de0d9bbd6b
--- /dev/null
+++ b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h
@@ -0,0 +1,17 @@
+#ifndef OVERLOADED_DELETE_IN_HEADER
+#define OVERLOADED_DELETE_IN_HEADER
+
+void clang_analyzer_printState();
+
+struct DeleteInHeader {
+  inline void operator delete(void *ptr) {
+    // No matter whether this header file is included as a system header file
+    // with -isystem or a user header file with -I, ptr should not be marked as
+    // released.
+    clang_analyzer_printState();
+
+    ::operator delete(ptr); // The first place where ptr is marked as released.
+  }
+};
+
+#endif // OVERLOADED_DELETE_IN_SYSTEM_HEADER
diff --git a/clang/test/Analysis/overloaded-delete-in-system-header.cpp b/clang/test/Analysis/overloaded-delete-in-system-header.cpp
new file mode 100644
index 00000000000000..f7780b67e93b99
--- /dev/null
+++ b/clang/test/Analysis/overloaded-delete-in-system-header.cpp
@@ -0,0 +1,25 @@
+// issue 62985
+// When 3rd-party header files are included as system headers, their overloaded
+// new and delete operators are also considered as the std ones. However, those
+// overloaded operator functions will also be inlined. This makes the same
+// symbolic memory marked as released twice, which leads to a false uaf alarm.
+//
+// The first run, include as system header. False uaf report before fix.
+//
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \
+// RUN:   -isystem %S/Inputs/ 2>&1 | \
+// RUN:   FileCheck %s
+//
+// The second run, include as user header. Should always silent.
+//
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \
+// RUN:   -I %S/Inputs/ 2>&1 | \
+// RUN:   FileCheck %s
+
+#include "overloaded-delete-in-header.h"
+
+void deleteInHeader(DeleteInHeader *p) { delete p; }
+
+// CHECK-NOT: Released

[Snape3058]

This version is just a trivial workaround for this issue. Refer to the FIXME comment in the checker. Feel free to provide suggestions on fixing this bug.

[steakhal]

This check still has some problems.
FD is not guaranteed to be the function decl which has the body, thus consequently, the location of FD might not point to the definition, even if FD has a definition.

FD likely points to the last decl declaration spelling.
I didn't have time to publish my extra tests demonstrating this, but I'll come back to this.

[Snape3058]

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.

[balazske]

Is it possible to move the check into checkPostCall? Then this situation can not happen.

[steakhal]

This is an excellent question! :D

[steakhal]

I had a deeper look at the patch and I think it's good.
To be on the safe side, I canonicalize FD to prefer decls with definitions if the redeclchain contains a definition.
This ensures that if we have a definition, L will refer to the beginning of that definition.

I'll merge this PR in a couple of days unless someone objects, or you challenge my proposed changes @Snape3058.
Thanks for the PR btw.

[nathanjsmith]

Any update on whether this can be merged? The false positive detection is preventing me from upgrading beyond clang 14.

[NagyDonat]

It seems that we forgot about this issue 😓, sorry for the trouble!

The change looks good to me at first glance, although I didn't investigate the context and the the possible indirect effects.