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..f42689218bb1a 100644 --- a/clang/unittests/StaticAnalyzer/CallEventTest.cpp +++ b/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( + Code, {"-x", "objective-c", "-Wno-objc-root-class"}, Diags)); + } +} + } // namespace } // namespace ento } // namespace clang