Skip to content

Commit 8a93c5b

Browse files
sam-mccallbnbarham
authored andcommitted
[clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing
In principle it's OK for stdlib-indexing tasks to run after the TUScheduler is destroyed, as mostly they just update the dynamic index. We do drain the stdlib-indexing queue before destroying the index. However the task captures references to the PreambleCallbacks object, which is owned by the TUScheduler. Once this is destroyed (explicitly, early in ~ClangdServer) an outstanding stdlib-indexing task may use-after-free. The fix here is to avoid capturing references to the PreambleCallbacks. Alternatives would be to have TUScheduler (exclusively) not own its callbacks so they could live longer, or explicitly stopping the TUScheduler instead of early-destroying it. These both seem more invasive. See https://reviews.llvm.org/D115232 for some more context. Differential Revision: https://reviews.llvm.org/D140486
1 parent eeb1938 commit 8a93c5b

File tree

1 file changed

+16
-7
lines changed

1 file changed

+16
-7
lines changed

clang-tools-extra/clangd/ClangdServer.cpp

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,27 +63,36 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
6363
ClangdServer::Callbacks *ServerCallbacks,
6464
const ThreadsafeFS &TFS, AsyncTaskRunner *Tasks)
6565
: FIndex(FIndex), ServerCallbacks(ServerCallbacks), TFS(TFS),
66-
Tasks(Tasks) {}
66+
Stdlib{std::make_shared<StdLibSet>()}, Tasks(Tasks) {}
6767

6868
void onPreambleAST(PathRef Path, llvm::StringRef Version,
6969
const CompilerInvocation &CI, ASTContext &Ctx,
7070
Preprocessor &PP,
7171
const CanonicalIncludes &CanonIncludes) override {
7272
// If this preamble uses a standard library we haven't seen yet, index it.
7373
if (FIndex)
74-
if (auto Loc = Stdlib.add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
74+
if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
7575
indexStdlib(CI, std::move(*Loc));
7676

7777
if (FIndex)
7878
FIndex->updatePreamble(Path, Version, Ctx, PP, CanonIncludes);
7979
}
8080

8181
void indexStdlib(const CompilerInvocation &CI, StdLibLocation Loc) {
82-
auto Task = [this, LO(*CI.getLangOpts()), Loc(std::move(Loc)),
83-
CI(std::make_unique<CompilerInvocation>(CI))]() mutable {
82+
// This task is owned by Tasks, which outlives the TUScheduler and
83+
// therefore the UpdateIndexCallbacks.
84+
// We must be careful that the references we capture outlive TUScheduler.
85+
auto Task = [LO(*CI.getLangOpts()), Loc(std::move(Loc)),
86+
CI(std::make_unique<CompilerInvocation>(CI)),
87+
// External values that outlive ClangdServer
88+
TFS(&TFS),
89+
// Index outlives TUScheduler (declared first)
90+
FIndex(FIndex),
91+
// shared_ptr extends lifetime
92+
Stdlib(Stdlib)]() mutable {
8493
IndexFileIn IF;
85-
IF.Symbols = indexStandardLibrary(std::move(CI), Loc, TFS);
86-
if (Stdlib.isBest(LO))
94+
IF.Symbols = indexStandardLibrary(std::move(CI), Loc, *TFS);
95+
if (Stdlib->isBest(LO))
8796
FIndex->updatePreamble(std::move(IF));
8897
};
8998
if (Tasks)
@@ -128,7 +137,7 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
128137
FileIndex *FIndex;
129138
ClangdServer::Callbacks *ServerCallbacks;
130139
const ThreadsafeFS &TFS;
131-
StdLibSet Stdlib;
140+
std::shared_ptr<StdLibSet> Stdlib;
132141
AsyncTaskRunner *Tasks;
133142
};
134143

0 commit comments

Comments
 (0)