Author: Sam McCall Date: 2022-12-21T23:35:48+01:00 New Revision: b494f67f67968c50289148fb423a108607be83e9
URL: https://github.com/llvm/llvm-project/commit/b494f67f67968c50289148fb423a108607be83e9 DIFF: https://github.com/llvm/llvm-project/commit/b494f67f67968c50289148fb423a108607be83e9.diff LOG: [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 Added: Modified: clang-tools-extra/clangd/ClangdServer.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 9c8803bbfb1ce..349a53d4dc14e 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -63,7 +63,7 @@ struct UpdateIndexCallbacks : public ParsingCallbacks { ClangdServer::Callbacks *ServerCallbacks, const ThreadsafeFS &TFS, AsyncTaskRunner *Tasks) : FIndex(FIndex), ServerCallbacks(ServerCallbacks), TFS(TFS), - Tasks(Tasks) {} + Stdlib{std::make_shared<StdLibSet>()}, Tasks(Tasks) {} void onPreambleAST(PathRef Path, llvm::StringRef Version, const CompilerInvocation &CI, ASTContext &Ctx, @@ -71,7 +71,7 @@ struct UpdateIndexCallbacks : public ParsingCallbacks { const CanonicalIncludes &CanonIncludes) override { // If this preamble uses a standard library we haven't seen yet, index it. if (FIndex) - if (auto Loc = Stdlib.add(*CI.getLangOpts(), PP.getHeaderSearchInfo())) + if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo())) indexStdlib(CI, std::move(*Loc)); if (FIndex) @@ -79,11 +79,20 @@ struct UpdateIndexCallbacks : public ParsingCallbacks { } void indexStdlib(const CompilerInvocation &CI, StdLibLocation Loc) { - auto Task = [this, LO(*CI.getLangOpts()), Loc(std::move(Loc)), - CI(std::make_unique<CompilerInvocation>(CI))]() mutable { + // This task is owned by Tasks, which outlives the TUScheduler and + // therefore the UpdateIndexCallbacks. + // We must be careful that the references we capture outlive TUScheduler. + auto Task = [LO(*CI.getLangOpts()), Loc(std::move(Loc)), + CI(std::make_unique<CompilerInvocation>(CI)), + // External values that outlive ClangdServer + TFS(&TFS), + // Index outlives TUScheduler (declared first) + FIndex(FIndex), + // shared_ptr extends lifetime + Stdlib(Stdlib)]() mutable { IndexFileIn IF; - IF.Symbols = indexStandardLibrary(std::move(CI), Loc, TFS); - if (Stdlib.isBest(LO)) + IF.Symbols = indexStandardLibrary(std::move(CI), Loc, *TFS); + if (Stdlib->isBest(LO)) FIndex->updatePreamble(std::move(IF)); }; if (Tasks) @@ -128,7 +137,7 @@ struct UpdateIndexCallbacks : public ParsingCallbacks { FileIndex *FIndex; ClangdServer::Callbacks *ServerCallbacks; const ThreadsafeFS &TFS; - StdLibSet Stdlib; + std::shared_ptr<StdLibSet> Stdlib; AsyncTaskRunner *Tasks; }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits