From b7a56b629883654252e2eed28d708b343cb627cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= Date: Wed, 13 Dec 2023 10:35:39 +0100 Subject: [PATCH 1/2] [clang] Clear ObjCMethodCall's cache between runs `lookupRuntimeDefinition` assumed that a process would handle only one TU. This is not true for unit tests, for instance. Multiple snippets of code would get parsed, and their AST would be 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 --- .../Core/PathSensitive/CallEvent.h | 2 +- clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 31 +++++++------- .../StaticAnalyzer/CallEventTest.cpp | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h index 5dcf03f7a4648..c233ca1af0256 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ b/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, diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp index 180056cf68b64..06ba01507fa4f 100644 --- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -1254,6 +1254,15 @@ template <> struct DenseMapInfo { }; } // 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>; +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>; - - static PrivateMethodCache PMC; std::optional &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, diff --git a/clang/unittests/StaticAnalyzer/CallEventTest.cpp b/clang/unittests/StaticAnalyzer/CallEventTest.cpp index 8b5289ea7472b..9015afdae87fc 100644 --- a/clang/unittests/StaticAnalyzer/CallEventTest.cpp +++ b/clang/unittests/StaticAnalyzer/CallEventTest.cpp @@ -84,6 +84,46 @@ 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( + Code, {"-x", "objective-c", "-Wno-objc-root-class"}, Diags)); + } +} + } // namespace } // namespace ento } // namespace clang From 591542a437fc873affa7129d38413df59670dd36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= Date: Tue, 30 Sep 2025 09:21:04 +0200 Subject: [PATCH 2/2] Format --- clang/unittests/StaticAnalyzer/CallEventTest.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clang/unittests/StaticAnalyzer/CallEventTest.cpp b/clang/unittests/StaticAnalyzer/CallEventTest.cpp index 9015afdae87fc..f42689218bb1a 100644 --- a/clang/unittests/StaticAnalyzer/CallEventTest.cpp +++ b/clang/unittests/StaticAnalyzer/CallEventTest.cpp @@ -85,9 +85,10 @@ TEST(CXXDeallocatorCall, SimpleDestructor) { } 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. + // 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;