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

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Ella Ma (Snape3058)

<details>
<summary>Changes</summary>

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

``````````

</details>


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