https://github.com/jansvoboda11 created https://github.com/llvm/llvm-project/pull/196117
Writing to the module cache would invalidate the read buffer. If the timing works out just right, this is a use-after-free bug. This PR prevents that situation by using two buffers in the module cache entry, and adds a unit test that would previously fail under address sanitizer. >From 0cd822384e3fd9e9033cc38723ac6ba538f10e77 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <[email protected]> Date: Wed, 6 May 2026 09:50:04 -0700 Subject: [PATCH] [clang][modules] Fix UAF in `InProcessModuleCache` Writing to the module cache would invalidate the read buffer. If the timing works out just right, this is a use-after-free bug. This PR prevents that situation by using two buffers in the module cache entry, and adds a unit test that would previously fail under address sanitizer. --- .../DependencyScanning/InProcessModuleCache.h | 6 +- .../InProcessModuleCache.cpp | 25 ++++--- .../DependencyScanning/CMakeLists.txt | 1 + .../InProcessModuleCacheTest.cpp | 72 +++++++++++++++++++ 4 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 clang/unittests/DependencyScanning/InProcessModuleCacheTest.cpp diff --git a/clang/include/clang/DependencyScanning/InProcessModuleCache.h b/clang/include/clang/DependencyScanning/InProcessModuleCache.h index de76cbe3f3ad0..61b56adaa80a0 100644 --- a/clang/include/clang/DependencyScanning/InProcessModuleCache.h +++ b/clang/include/clang/DependencyScanning/InProcessModuleCache.h @@ -37,8 +37,10 @@ struct ModuleCacheEntry { S_Read, S_Written, } State = S_Unknown; - /// The buffer that we've either read from disk or written in-process. - std::unique_ptr<llvm::MemoryBuffer> Buffer; + /// The buffer we've read from disk, if any. + std::unique_ptr<llvm::MemoryBuffer> ReadBuffer; + /// The buffer we've written to module cache, if any. + std::unique_ptr<llvm::MemoryBuffer> WrittenBuffer; /// The modification time of the entry. time_t ModTime = 0; }; diff --git a/clang/lib/DependencyScanning/InProcessModuleCache.cpp b/clang/lib/DependencyScanning/InProcessModuleCache.cpp index 538ff2952c4f5..ae858e5d2bda4 100644 --- a/clang/lib/DependencyScanning/InProcessModuleCache.cpp +++ b/clang/lib/DependencyScanning/InProcessModuleCache.cpp @@ -24,13 +24,14 @@ void ModuleCacheEntries::flush() { auto BypassSandbox = llvm::sys::sandbox::scopedDisable(); for (auto &[Path, Entry] : Map) { if (Entry->State == ModuleCacheEntry::S_Written) { + assert(Entry->WrittenBuffer && "Wrote PCM with no contents"); // Note: We could propagate Entry->ModTime to the on-disk file, but // implicitly-built modules (unlike explicitly-built modules) don't use // that metadata to refer to imports, rendering this unnecessary. off_t Size; time_t ModTime; // Best-effort: ignore errors (e.g. read-only cache directory). - (void)writeImpl(Path, Entry->Buffer->getMemBufferRef(), Size, ModTime); + (void)writeImpl(Path, *Entry->WrittenBuffer, Size, ModTime); } } } @@ -145,18 +146,18 @@ class InProcessModuleCache : public ModuleCache { ModuleCacheEntry &Entry = getOrCreateEntry(Path); std::lock_guard<std::mutex> Lock(Entry.Mutex); if (Entry.State == ModuleCacheEntry::S_Written) { - assert(Entry.Buffer && "Wrote PCM with no contents"); - assert(Entry.Buffer->getBuffer() == Buffer.getBuffer() && + assert(Entry.WrittenBuffer && "Wrote PCM with no contents"); + assert(Entry.WrittenBuffer->getBuffer() == Buffer.getBuffer() && "Wrote the same PCM with different contents"); - Size = Entry.Buffer->getBufferSize(); + Size = Entry.WrittenBuffer->getBufferSize(); ModTime = Entry.ModTime; return {}; } - Entry.Buffer = + Entry.WrittenBuffer = llvm::MemoryBuffer::getMemBufferCopy(Buffer.getBuffer(), Path); Entry.ModTime = llvm::sys::toTimeT(std::chrono::system_clock::now()); Entry.State = ModuleCacheEntry::S_Written; - Size = Entry.Buffer->getBufferSize(); + Size = Entry.WrittenBuffer->getBufferSize(); ModTime = Entry.ModTime; return {}; } @@ -173,14 +174,18 @@ class InProcessModuleCache : public ModuleCache { auto ReadBuffer = readImpl(FileName, ReadSize, ReadModTime); if (!ReadBuffer) return ReadBuffer.takeError(); - Entry.Buffer = std::move(*ReadBuffer); + Entry.ReadBuffer = std::move(*ReadBuffer); Entry.ModTime = ReadModTime; Entry.State = ModuleCacheEntry::S_Read; } - Size = Entry.Buffer->getBufferSize(); + // The written buffer takes precedence over any read buffer. + llvm::MemoryBuffer *Buffer = Entry.WrittenBuffer ? Entry.WrittenBuffer.get() + : Entry.ReadBuffer.get(); + Size = Buffer->getBufferSize(); ModTime = Entry.ModTime; - return llvm::MemoryBuffer::getMemBuffer(*Entry.Buffer, - /* RequiresNullTerminator */ false); + // Note: Creates a reference to ReadBuffer or WrittenBuffer. + return llvm::MemoryBuffer::getMemBuffer(*Buffer, + /*RequiresNullTerminator=*/false); } }; } // namespace diff --git a/clang/unittests/DependencyScanning/CMakeLists.txt b/clang/unittests/DependencyScanning/CMakeLists.txt index 3e7f902483968..4f2a36067209c 100644 --- a/clang/unittests/DependencyScanning/CMakeLists.txt +++ b/clang/unittests/DependencyScanning/CMakeLists.txt @@ -1,5 +1,6 @@ add_clang_unittest(ClangDependencyScanningTests DependencyScanningFilesystemTest.cpp + InProcessModuleCacheTest.cpp CLANG_LIBS clangDependencyScanning clangFrontend # For TextDiagnosticPrinter. diff --git a/clang/unittests/DependencyScanning/InProcessModuleCacheTest.cpp b/clang/unittests/DependencyScanning/InProcessModuleCacheTest.cpp new file mode 100644 index 0000000000000..0c6435bce773c --- /dev/null +++ b/clang/unittests/DependencyScanning/InProcessModuleCacheTest.cpp @@ -0,0 +1,72 @@ +#include "clang/DependencyScanning/InProcessModuleCache.h" + +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/MemoryBuffer.h" +#include "llvm/Testing/Support/Error.h" + +#include "gtest/gtest.h" + +using namespace clang; +using namespace clang::dependencies; + +TEST(InProcessModuleCache, ReadWriteInvalidation) { + ModuleCacheEntries Entries; + std::shared_ptr<ModuleCache> ModCache = makeInProcessModuleCache(Entries); + + int FD; + llvm::SmallString<256> Path; + ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("M", "pcm", FD, Path)); + + { + llvm::raw_fd_ostream OS(FD, /*shouldClose=*/true); + OS << "original\n"; + } + + off_t Size; + time_t ModTime; + std::unique_ptr<llvm::MemoryBuffer> Buf; + ASSERT_THAT_ERROR(ModCache->read(Path, Size, ModTime).moveInto(Buf), + llvm::Succeeded()); + EXPECT_EQ(Buf->getBuffer(), "original\n"); + + auto NewBuf = llvm::MemoryBuffer::getMemBufferCopy("modified\n", Path); + ASSERT_EQ(ModCache->write(Path, *NewBuf, Size, ModTime), std::error_code{}); + + // Writing a new buffer should not invalidate the previously-read buffer. + EXPECT_EQ(Buf->getBuffer(),"original\n"); +} + + +TEST(InProcessModuleCache, ReadReadInvalidation) { + ModuleCacheEntries Entries; + std::shared_ptr<ModuleCache> ModCache = makeInProcessModuleCache(Entries); + + int FD; + llvm::SmallString<256> Path; + ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("M", "pcm", FD, Path)); + + { + llvm::raw_fd_ostream OS(FD, /*shouldClose=*/true); + OS << "original\n"; + } + + off_t Size1; + time_t ModTime1; + std::unique_ptr<llvm::MemoryBuffer> Buf1; + ASSERT_THAT_ERROR(ModCache->read(Path, Size1, ModTime1).moveInto(Buf1), + llvm::Succeeded()); + EXPECT_EQ(Buf1->getBuffer(), "original\n"); + + off_t Size2; + time_t ModTime2; + std::unique_ptr<llvm::MemoryBuffer> Buf2; + ASSERT_THAT_ERROR(ModCache->read(Path, Size2, ModTime2).moveInto(Buf2), + llvm::Succeeded()); + EXPECT_EQ(Buf2->getBuffer(), "original\n"); + + // Subsequent reads should not invalidate previous reads. + EXPECT_EQ(Buf1->getBuffer(), "original\n"); + // All read buffers should point to the same memory. + EXPECT_EQ(Buf1->getBuffer().begin(), Buf2->getBuffer().begin()); + EXPECT_EQ(Buf1->getBuffer().end(), Buf2->getBuffer().end()); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
