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

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd03a7f15f019: [clangd] SIGSEGV at clangd: DiagnosticConsumer 
Is Used After Free (authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, 
std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

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::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
  clang::declvisitor::Base::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
  clang::declvisitor::Base::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::indexTopLevelDecls(clang::ASTContext&, clang::Preprocessor&, 
llvm::ArrayRef, clang::index::IndexDataConsumer&, 
clang::index::IndexingOptions)
  clang::clangd::(anonymous namespace)::indexSymbols(clang::ASTContext&, 
std::shared_ptr, llvm::ArrayRef, 
clang::clangd::MainFileMacros const*, clang::clangd::CanonicalIncludes const&, 
bool, llvm::StringRef, bool)
  clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, 
std::shared_ptr, clang::clangd::CanonicalIncludes const&)
  clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, 
clang::ASTContext&, std::shared_ptr, 
clang::clangd::CanonicalIncludes const&)
  void
  void
  threadFuncAsync(void*)
  start_thread


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:709
   Ctx->setStatCache(Result->StatCache);
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed

sammccall wrote:
> 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.
I applied the change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 555441.
ivanmurashko marked an inline comment as done.
ivanmurashko added a comment.

@sammccall's comment was addressed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, 
std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:610-619
   StoreDiags PreambleDiagnostics;
   PreambleDiagnostics.setDiagCallback(
   [](const clang::Diagnostic , clangd::Diag ) {
 llvm::for_each(ASTListeners,
[&](const auto ) { L->sawDiagnostic(D, Diag); });
   });
   llvm::IntrusiveRefCntPtr PreambleDiagsEngine =

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice catch!




Comment at: clang-tools-extra/clangd/Preamble.cpp:709
   Ctx->setStatCache(Result->StatCache);
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:610-619
   StoreDiags PreambleDiagnostics;
   PreambleDiagnostics.setDiagCallback(
   [](const clang::Diagnostic , clangd::Diag ) {
 llvm::for_each(ASTListeners,
[&](const auto ) { L->sawDiagnostic(D, Diag); });
   });
   llvm::IntrusiveRefCntPtr PreambleDiagsEngine =

Alternative fix


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159363/new/

https://reviews.llvm.org/D159363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Murashko via Phabricator via cfe-commits]

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

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 PreambleDiagsEngine =
  CompilerInstance::createDiagnostics((),
  ,
  /*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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -706,6 +706,11 @@
   // 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;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -706,6 +706,11 @@
   // 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;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits