[clang-tools-extra] d03a7f1 - [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free
Ivan Murashko via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 4 10:54:06 PDT 2023
Author: Ivan Murashko
Date: 2023-09-04T18:53:49+01:00
New Revision: d03a7f15f019beb1896872ba9321cfed5f16a05f
URL: https://github.com/llvm/llvm-project/commit/d03a7f15f019beb1896872ba9321cfed5f16a05f
DIFF: https://github.com/llvm/llvm-project/commit/d03a7f15f019beb1896872ba9321cfed5f16a05f.diff
LOG: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free
This is a follow-up patch for D148088. The dynamic symbol index (`FileIndex::updatePreamble`) may run in a separate thread, and the `DiagnosticConsumer` that is set up in `buildPreamble` might go out of scope before it is used. This could result in a SIGSEGV when attempting to call any method of the `DiagnosticConsumer` class.
The function `buildPreamble` sets up the `DiagnosticConsumer` as follows:
```
... buildPreamble(...) {
...
StoreDiags PreambleDiagnostics;
...
llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
&PreambleDiagnostics,
/*ShouldOwnClient=*/false);
...
// The call might use the diagnostic consumer in a separate thread
PreambleCallback(...)
...
}
```
`PreambleDiagnostics` might be out of scope for `buildPreamble` function when we call it inside `PreambleCallback` in a separate thread.
The Fix
The fix involves replacing the client (DiagnosticConsumer) with an `IgnoringDiagConsumer` instance, which will print messages to the clangd log.
Alternatively, we can replace `PreambleDiagnostics` with an object that is owned by `DiagnosticsEngine`.
Note
There is no corresponding LIT/GTest for this issue, since there is a specific race condition that is difficult to reproduce within a test framework.
Test Plan:
```
ninja check-clangd
```
Reviewed By: kadircet, sammccall
Differential Revision: https://reviews.llvm.org/D159363
Added:
Modified:
clang-tools-extra/clangd/Preamble.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 725e6b1e34fea3..f8fd4327681705 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
Stats ? TimedFS : StatCacheFS, std::make_shared<PCHContainerOperations>(),
StoreInMemory, /*StoragePath=*/"", CapturedInfo);
PreambleTimer.stopTimer();
+
+ // We have to setup DiagnosticConsumer that will be alife
+ // while preamble callback is executed
+ PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
// Reset references to ref-counted-ptrs before executing the callbacks, to
// prevent resetting them concurrently.
PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
// While extending the life of FileMgr and VFS, StatCache should also be
// extended.
Ctx->setStatCache(Result->StatCache);
+
PreambleCallback(std::move(*Ctx), Result->Pragmas);
}
return Result;
More information about the cfe-commits
mailing list