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

[via cfe-commits]

Author: Ella Ma
Date: 2024-10-31T17:02:28+01:00
New Revision: f4af60dfbb6a2e3d5628b8f07b4895ddbe24d459

URL: https://github.com/llvm/llvm-project/commit/f4af60dfbb6a2e3d5628b8f07b4895ddbe24d459
DIFF: https://github.com/llvm/llvm-project/commit/f4af60dfbb6a2e3d5628b8f07b4895ddbe24d459.diff

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

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.

Added: 
    clang/test/Analysis/Inputs/overloaded-delete-in-header.h
    clang/test/Analysis/overloaded-delete-in-system-header.cpp

Modified: 
    clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3e95db7e97fac8..4166cf14391e2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1091,12 +1091,15 @@ static bool isStandardDelete(const FunctionDecl *FD) {
   if (Kind != OO_Delete && Kind != OO_Array_Delete)
     return false;
 
+  bool HasBody = FD->hasBody(); // Prefer using the definition.
+
   // This is standard if and only if it's not defined in a user file.
   SourceLocation L = FD->getLocation();
+
   // 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);
+  const auto &SM = FD->getASTContext().getSourceManager();
+  return L.isInvalid() || (!HasBody && SM.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..8243961d84830b
--- /dev/null
+++ b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h
@@ -0,0 +1,18 @@
+#ifndef OVERLOADED_DELETE_IN_HEADER
+#define OVERLOADED_DELETE_IN_HEADER
+
+struct DeleteInHeader {
+  int data;
+  static void operator delete(void *ptr);
+};
+
+void DeleteInHeader::operator delete(void *ptr) {
+  DeleteInHeader *self = (DeleteInHeader *)ptr;
+  self->data = 1; // no-warning: Still alive.
+
+  ::operator delete(ptr);
+
+  self->data = 2; // expected-warning {{Use of memory after it is freed [cplusplus.NewDelete]}}
+}
+
+#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..c284a942063061
--- /dev/null
+++ b/clang/test/Analysis/overloaded-delete-in-system-header.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_analyze_cc1 -isystem %S/Inputs/ -verify %s \
+// RUN:   -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete
+
+// RUN: %clang_analyze_cc1 -I %S/Inputs/ -verify %s \
+// RUN:   -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete
+
+#include "overloaded-delete-in-header.h"
+
+void deleteInHeader(DeleteInHeader *p) { delete p; }