[clang-tools-extra] b494f67 - [clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing

[Sam McCall via cfe-commits]

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;
 };