[PATCH] D140486: [clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140486

Files:
  clang-tools-extra/clangd/ClangdServer.cpp


Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -63,7 +63,7 @@
                        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 @@
                      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,22 @@
   }
 
   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 = [
+                    // Captured by value
+                    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 +139,7 @@
   FileIndex *FIndex;
   ClangdServer::Callbacks *ServerCallbacks;
   const ThreadsafeFS &TFS;
-  StdLibSet Stdlib;
+  std::shared_ptr<StdLibSet> Stdlib;
   AsyncTaskRunner *Tasks;
 };
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D140486.484612.patch
Type: text/x-patch
Size: 2498 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221221/909a06db/attachment.bin>