[clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[ivanmurashko]

Summary

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

[ivanmurashko]

clang-tools-extra/clangd/Preamble.cpp

610–619

Alternative fix

[sammccall]

Nice catch!

clang-tools-extra/clangd/Preamble.cpp

709

I think this should go up next to PreambleDiagsEngine.reset() etc, as it's basically the same thing: we're trying to avoid any race between the async work done by the callback and the cleanup at the end of the function.

Also I think it's slightly clearer for us to consistently be taking the diagnostics from PreambleDiagnostics *after* it's detached from the compiler.

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

610–619

I prefer the version in the patch: we keep PreambleDiagnostics auto-scoped and can more easily reason about its lifetime.

[ivanmurashko]

@sammccall's comment was addressed

clang-tools-extra/clangd/Preamble.cpp

709

I applied the change

[kadircet]

thanks, the fix LGTM as well.

but i wonder how this surfaces, to make sure we're taking necessary precautions in the future. we definitely have a dangling reference, which isn't great. but it's surprising that we access diags consumer during indexing.
I assume it's about the modules setup you're running clangd in. Do you have any stack traces that shows the execution path? my assumption is, this triggers when clangd ends up deserializing some symbols from a module. If these end up being important diagnostics, we might want to figure out how to emit diagnostics from these stages as well.

[kadircet]

This comment was removed by kadircet.

[ivanmurashko]

In D159363#4636581, @kadircet wrote:

thanks, the fix LGTM as well.

but i wonder how this surfaces, to make sure we're taking necessary precautions in the future. we definitely have a dangling reference, which isn't great. but it's surprising that we access diags consumer during indexing.
I assume it's about the modules setup you're running clangd in. Do you have any stack traces that shows the execution path? my assumption is, this triggers when clangd ends up deserializing some symbols from a module. If these end up being important diagnostics, we might want to figure out how to emit diagnostics from these stages as well.

Yes, you are right. The diags consumer is triggered when it tries to read an implicit module that has some incompatibilities with the preamble headers.

The typical stack trace is below (that is LLVM-12 specific)

clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&)
clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool)
clang::DiagnosticBuilder::~DiagnosticBuilder()
clang::ASTReader::diagnoseOdrViolations()
clang::ASTReader::FinishedDeserializing()
clang::DeclContext::LoadLexicalDeclsFromExternalStorage()
clang::DeclContext::decls_begin()
clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
clang::declvisitor::Base<llvm::make_const_ptr, (anonymous namespace)::IndexingDeclVisitor, bool>::Visit(clang::Decl const*)
clang::index::IndexingContext::indexDecl(clang::Decl const*)
clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
clang::declvisitor::Base<llvm::make_const_ptr, (anonymous namespace)::IndexingDeclVisitor, bool>::Visit(clang::Decl const*)
clang::index::IndexingContext::indexDecl(clang::Decl const*)
clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
clang::declvisitor::Base<llvm::make_const_ptr, (anonymous namespace)::IndexingDeclVisitor, bool>::Visit(clang::Decl const*)
clang::index::IndexingContext::indexDecl(clang::Decl const*)
clang::index::indexTopLevelDecls(clang::ASTContext&, clang::Preprocessor&, llvm::ArrayRef<clang::Decl const*>, clang::index::IndexDataConsumer&, clang::index::IndexingOptions)
clang::clangd::(anonymous namespace)::indexSymbols(clang::ASTContext&, std::shared_ptr<clang::Preprocessor>, llvm::ArrayRef<clang::Decl*>, clang::clangd::MainFileMacros const*, clang::clangd::CanonicalIncludes const&, bool, llvm::StringRef, bool)
clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, std::shared_ptr<clang::Preprocessor>, clang::clangd::CanonicalIncludes const&)
clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, clang::ASTContext&, std::shared_ptr<clang::Preprocessor>, clang::clangd::CanonicalIncludes const&)
void
void
threadFuncAsync(void*)
start_thread

clang-tools-extra/clangd/Preamble.cpp

buildPreamble(PathRef FileName, CompilerInvocation CI,
   SPAN_ATTACH(Tracer, "File", FileName);
   std::vector<std::unique_ptr<FeatureModule::ASTListener>> ASTListeners;
   if (Inputs.FeatureModules) {
     for (auto &M : *Inputs.FeatureModules) {
       if (auto Listener = M.astListeners())
         ASTListeners.emplace_back(std::move(Listener));
     }
   }
   StoreDiags PreambleDiagnostics;
   PreambleDiagnostics.setDiagCallback(
       [&ASTListeners](const clang::Diagnostic &D, clangd::Diag &Diag) {
         llvm::for_each(ASTListeners,
                        [&](const auto &L) { L->sawDiagnostic(D, Diag); });
       });
   llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnostics,
                                           /*ShouldOwnClient=*/false);
   const Config &Cfg = Config::current();
   PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
     if (Cfg.Diagnostics.SuppressAll ||
         isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
                                       *CI.getLangOpts()))
       return DiagnosticsEngine::Ignored;
     switch (Info.getID()) {
if (PreambleCallback) {
       auto Ctx = CapturedInfo.takeLife();
       // Stat cache is thread safe only when there are no producers. Hence
       // change the VFS underneath to a consuming fs.
       Ctx->getFileManager().setVirtualFileSystem(
           Result->StatCache->getConsumingFS(VFS));
       // While extending the life of FileMgr and VFS, StatCache should also be
       // extended.
       Ctx->setStatCache(Result->StatCache);
+      // We have to setup DiagnosticConsumer that will be alife
+      // while preamble callback is executed
+      Ctx->getASTContext().getDiagnostics().setClient(new IgnoringDiagConsumer,
+                                                      true);
+
       PreambleCallback(std::move(*Ctx), Result->Pragmas);
     }
     return Result;
   }
 
   elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
        Inputs.Version, BuiltPreamble.getError().message());
   for (const Diag &D : PreambleDiagnostics.take()) {