[clang][analyzer] Clear ObjCMethodCall's cache between runs (#161327)

`lookupRuntimeDefinition` assumed that a process would handle only one
TU. This is not true for unit tests, for instance. Multiple snippets of
code get parsed, and their AST are unloaded each time.

Since the cache relies on pointers as keys, if the same address happens
to be reused between runs, the cache would return a stale pointer,
potentially causing a segmentation fault. This is not that unlikely if
the snippets are similar, which would trigger similar allocation
patterns.

CPP-4889

Author: alejandro-alvarez-sonarsource

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

@@ -1414,7 +1414,7 @@ class CallEventManager {
   }
 
 public:
-  CallEventManager(llvm::BumpPtrAllocator &alloc) : Alloc(alloc) {}
+  CallEventManager(llvm::BumpPtrAllocator &alloc);
 
   /// Gets an outside caller given a callee context.
   CallEventRef<> getCaller(const StackFrameContext *CalleeCtx,

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

@@ -1254,6 +1254,15 @@ template <> struct DenseMapInfo<PrivateMethodKey> {
 };
 } // end namespace llvm
 
+// NOTE: This cache is a "global" variable, and it is cleared by
+// CallEventManager's constructor so we do not keep old entries when
+// loading/unloading ASTs. If we are worried about concurrency, we may  need to
+// revisit this someday. In terms of memory, this table stays around until clang
+// quits, which also may be bad if we need to release memory.
+using PrivateMethodCacheTy =
+    llvm::DenseMap<PrivateMethodKey, std::optional<const ObjCMethodDecl *>>;
+static PrivateMethodCacheTy PrivateMethodCache;
+
 static const ObjCMethodDecl *
 lookupRuntimeDefinition(const ObjCInterfaceDecl *Interface,
                         Selector LookupSelector, bool InstanceMethod) {
@@ -1262,21 +1271,8 @@ lookupRuntimeDefinition(const ObjCInterfaceDecl *Interface,
   // that repeated queries on the same ObjCIntefaceDecl and Selector
   // don't incur the same cost.  On some test cases, we can see the
   // same query being issued thousands of times.
-  //
-  // NOTE: This cache is essentially a "global" variable, but it
-  // only gets lazily created when we get here.  The value of the
-  // cache probably comes from it being global across ExprEngines,
-  // where the same queries may get issued.  If we are worried about
-  // concurrency, or possibly loading/unloading ASTs, etc., we may
-  // need to revisit this someday.  In terms of memory, this table
-  // stays around until clang quits, which also may be bad if we
-  // need to release memory.
-  using PrivateMethodCache =
-      llvm::DenseMap<PrivateMethodKey, std::optional<const ObjCMethodDecl *>>;
-
-  static PrivateMethodCache PMC;
   std::optional<const ObjCMethodDecl *> &Val =
-      PMC[{Interface, LookupSelector, InstanceMethod}];
+      PrivateMethodCache[{Interface, LookupSelector, InstanceMethod}];
 
   // Query lookupPrivateMethod() if the cache does not hit.
   if (!Val) {
@@ -1422,6 +1418,13 @@ void ObjCMethodCall::getInitialStackFrameContents(
   }
 }
 
+CallEventManager::CallEventManager(llvm::BumpPtrAllocator &alloc)
+    : Alloc(alloc) {
+  // Clear the method cache to avoid hits when multiple AST are loaded/unloaded
+  // within a single process. This can happen with unit tests, for instance.
+  PrivateMethodCache.clear();
+}
+
 CallEventRef<>
 CallEventManager::getSimpleCall(const CallExpr *CE, ProgramStateRef State,
                                 const LocationContext *LCtx,

clang/unittests/StaticAnalyzer/CallEventTest.cpp

@@ -84,6 +84,47 @@ TEST(CXXDeallocatorCall, SimpleDestructor) {
 #endif
 }
 
+TEST(PrivateMethodCache, NeverReturnDanglingPointersWithMultipleASTs) {
+  // Each iteration will load and unload an AST multiple times. Since the code
+  // is always the same, we increase the chance of hitting a bug in the private
+  // method cache, returning a dangling pointer and crashing the process. If the
+  // cache is properly cleared between runs, the test should pass.
+  for (int I = 0; I < 100; ++I) {
+    auto const *Code = R"(
+    typedef __typeof(sizeof(int)) size_t;
+
+    extern void *malloc(size_t size);
+    extern void *memcpy(void *dest, const void *src, size_t n);
+
+    @interface SomeMoreData {
+      char const* _buffer;
+      int _size;
+    }
+    @property(nonatomic, readonly) const char* buffer;
+    @property(nonatomic) int size;
+
+    - (void)appendData:(SomeMoreData*)other;
+
+    @end
+
+    @implementation SomeMoreData
+    @synthesize size = _size;
+    @synthesize buffer = _buffer;
+
+    - (void)appendData:(SomeMoreData*)other {
+      int const len = (_size + other.size); // implicit self._length
+      char* d = malloc(sizeof(char) * len);
+      memcpy(d + 20, other.buffer, len);
+    }
+
+    @end
+  )";
+    std::string Diags;
+    EXPECT_TRUE(runCheckerOnCodeWithArgs<addCXXDeallocatorChecker>(
+        Code, {"-x", "objective-c", "-Wno-objc-root-class"}, Diags));
+  }
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang