Author: ibiryukov Date: Fri Mar 2 04:28:27 2018 New Revision: 326569 URL: http://llvm.org/viewvc/llvm-project?rev=326569&view=rev Log: [clang] Fix use-after-free on code completion
Summary: Found by asan. Fiddling with code completion AST after FrontendAction::Exceute can lead to errors. Calling the callback in ProcessCodeCompleteResults to make sure we don't access uninitialized state. This particular issue comes from the fact that Sema::TUScope is deleted when destructor of ~Parser runs, but still present in Sema::TUScope and accessed when building completion items. I'm still struggling to come up with a small repro. The relevant stackframes reported by asan are: ERROR: AddressSanitizer: heap-use-after-free on address READ of size 8 at 0x61400020d090 thread T175 #0 0x5632dff7821b in llvm::SmallPtrSetImplBase::isSmall() const include/llvm/ADT/SmallPtrSet.h:195:33 #1 0x5632e0335901 in llvm::SmallPtrSetImplBase::insert_imp(void const*) include/llvm/ADT/SmallPtrSet.h:127:9 #2 0x5632e067347d in llvm::SmallPtrSetImpl<clang::Decl*>::insert(clang::Decl*) include/llvm/ADT/SmallPtrSet.h:372:14 #3 0x5632e065df80 in clang::Scope::AddDecl(clang::Decl*) tools/clang/include/clang/Sema/Scope.h:287:18 #4 0x5632e0623eea in clang::ASTReader::pushExternalDeclIntoScope(clang::NamedDecl*, clang::DeclarationName) clang/lib/Serialization/ASTReader.cpp #5 0x5632e062ce74 in clang::ASTReader::finishPendingActions() tools/clang/lib/Serialization/ASTReader.cpp:9164:9 .... #30 0x5632e02009c4 in clang::index::generateUSRForDecl(clang::Decl const*, llvm::SmallVectorImpl<char>&) tools/clang/lib/Index/USRGeneration.cpp:1037:6 #31 0x5632dff73eab in clang::clangd::(anonymous namespace)::getSymbolID(clang::CodeCompletionResult const&) tools/clang/tools/extra/clangd/CodeComplete.cpp:326:20 #32 0x5632dff6fe91 in clang::clangd::CodeCompleteFlow::mergeResults(std::vector<clang::CodeCompletionResult, std::allocator<clang::CodeCompletionResult> > const&, clang::clangd::SymbolSlab const&)::'lambda'(clang::CodeCompletionResult const&)::operator()(clang::CodeCompletionResult const&) tools/clang/tools/extra/clangd/CodeComplete.cpp:938:24 #33 0x5632dff6e426 in clang::clangd::CodeCompleteFlow::mergeResults(std::vector<clang::CodeCompletionResult, std::allocator<clang::CodeCompletionResult> > const&, clang::clangd::SymbolSlab const&) third_party/llvm/llvm/tools/clang/tools/extra/clangd/CodeComplete.cpp:949:38 #34 0x5632dff7a34d in clang::clangd::CodeCompleteFlow::runWithSema() llvm/tools/clang/tools/extra/clangd/CodeComplete.cpp:894:16 #35 0x5632dff6df6a in clang::clangd::CodeCompleteFlow::run(clang::clangd::(anonymous namespace)::SemaCompleteInput const&) &&::'lambda'()::operator()() const third_party/llvm/llvm/tools/clang/tools/extra/clangd/CodeComplete.cpp:858:35 #36 0x5632dff6cd42 in clang::clangd::(anonymous namespace)::semaCodeComplete(std::unique_ptr<clang::CodeCompleteConsumer, std::default_delete<clang::CodeCompleteConsumer> >, clang::CodeCompleteOptions const&, clang::clangd::(anonymous namespace)::SemaCompleteInput const&, llvm::function_ref<void ()>) tools/clang/tools/extra/clangd/CodeComplete.cpp:735:5 0x61400020d090 is located 80 bytes inside of 432-byte region [0x61400020d040,0x61400020d1f0) freed by thread T175 here: #0 0x5632df74e115 in operator delete(void*, unsigned long) projects/compiler-rt/lib/asan/asan_new_delete.cc:161:3 #1 0x5632e0b06973 in clang::Parser::~Parser() tools/clang/lib/Parse/Parser.cpp:410:3 #2 0x5632e0b06ddd in clang::Parser::~Parser() clang/lib/Parse/Parser.cpp:408:19 #3 0x5632e0b03286 in std::unique_ptr<clang::Parser, std::default_delete<clang::Parser> >::~unique_ptr() .../bits/unique_ptr.h:236:4 #4 0x5632e0b021c4 in clang::ParseAST(clang::Sema&, bool, bool) tools/clang/lib/Parse/ParseAST.cpp:182:1 #5 0x5632e0726544 in clang::FrontendAction::Execute() tools/clang/lib/Frontend/FrontendAction.cpp:904:8 #6 0x5632dff6cd05 in clang::clangd::(anonymous namespace)::semaCodeComplete(std::unique_ptr<clang::CodeCompleteConsumer, std::default_delete<clang::CodeCompleteConsumer> >, clang::CodeCompleteOptions const&, clang::clangd::(anonymous namespace)::SemaCompleteInput const&, llvm::function_ref<void ()>) tools/clang/tools/extra/clangd/CodeComplete.cpp:728:15 Reviewers: sammccall Reviewed By: sammccall Subscribers: klimek, jkorous-apple, cfe-commits, ioeric Differential Revision: https://reviews.llvm.org/D44000 Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=326569&r1=326568&r2=326569&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original) +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Mar 2 04:28:27 2018 @@ -429,13 +429,19 @@ std::vector<std::string> getQueryScopes( // It filters out ignored results (but doesn't apply fuzzy-filtering yet). // It doesn't do scoring or conversion to CompletionItem yet, as we want to // merge with index results first. +// Generally the fields and methods of this object should only be used from +// within the callback. struct CompletionRecorder : public CodeCompleteConsumer { - CompletionRecorder(const CodeCompleteOptions &Opts) + CompletionRecorder(const CodeCompleteOptions &Opts, + UniqueFunction<void()> ResultsCallback) : CodeCompleteConsumer(Opts.getClangCompleteOpts(), /*OutputIsBinary=*/false), CCContext(CodeCompletionContext::CCC_Other), Opts(Opts), CCAllocator(std::make_shared<GlobalCodeCompletionAllocator>()), - CCTUInfo(CCAllocator) {} + CCTUInfo(CCAllocator), ResultsCallback(std::move(ResultsCallback)) { + assert(this->ResultsCallback); + } + std::vector<CodeCompletionResult> Results; CodeCompletionContext CCContext; Sema *CCSema = nullptr; // Sema that created the results. @@ -466,6 +472,7 @@ struct CompletionRecorder : public CodeC continue; Results.push_back(Result); } + ResultsCallback(); } CodeCompletionAllocator &getAllocator() override { return *CCAllocator; } @@ -503,6 +510,7 @@ private: CodeCompleteOptions Opts; std::shared_ptr<GlobalCodeCompletionAllocator> CCAllocator; CodeCompletionTUInfo CCTUInfo; + UniqueFunction<void()> ResultsCallback; }; // Tracks a bounded number of candidates with the best scores. @@ -657,12 +665,10 @@ struct SemaCompleteInput { }; // Invokes Sema code completion on a file. -// Callback will be invoked once completion is done, but before cleaning up. bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, const clang::CodeCompleteOptions &Options, - const SemaCompleteInput &Input, - llvm::function_ref<void()> Callback = nullptr) { - auto Tracer = llvm::make_unique<trace::Span>("Sema completion"); + const SemaCompleteInput &Input) { + trace::Span Tracer("Sema completion"); std::vector<const char *> ArgStrs; for (const auto &S : Input.Command.CommandLine) ArgStrs.push_back(S.c_str()); @@ -729,12 +735,6 @@ bool semaCodeComplete(std::unique_ptr<Co log("Execute() failed when running codeComplete for " + Input.FileName); return false; } - Tracer.reset(); - - if (Callback) - Callback(); - - Tracer = llvm::make_unique<trace::Span>("Sema completion cleanup"); Action.EndSourceFile(); return true; @@ -813,7 +813,7 @@ clang::CodeCompleteOptions CodeCompleteO // other arenas, which must stay alive for us to build CompletionItems. // - we may get duplicate results from Sema and the Index, we need to merge. // -// So we start Sema completion first, but defer its cleanup until we're done. +// So we start Sema completion first, and do all our work in its callback. // We use the Sema context information to query the index. // Then we merge the two result sets, producing items that are Sema/Index/Both. // These items are scored, and the top N are synthesized into the LSP response. @@ -834,8 +834,7 @@ class CodeCompleteFlow { PathRef FileName; const CodeCompleteOptions &Opts; // Sema takes ownership of Recorder. Recorder is valid until Sema cleanup. - std::unique_ptr<CompletionRecorder> RecorderOwner; - CompletionRecorder &Recorder; + CompletionRecorder *Recorder = nullptr; int NSema = 0, NIndex = 0, NBoth = 0; // Counters for logging. bool Incomplete = false; // Would more be available with a higher limit? llvm::Optional<FuzzyMatcher> Filter; // Initialized once Sema runs. @@ -843,25 +842,24 @@ class CodeCompleteFlow { public: // A CodeCompleteFlow object is only useful for calling run() exactly once. CodeCompleteFlow(PathRef FileName, const CodeCompleteOptions &Opts) - : FileName(FileName), Opts(Opts), - RecorderOwner(new CompletionRecorder(Opts)), Recorder(*RecorderOwner) {} + : FileName(FileName), Opts(Opts) {} CompletionList run(const SemaCompleteInput &SemaCCInput) && { trace::Span Tracer("CodeCompleteFlow"); // We run Sema code completion first. It builds an AST and calculates: - // - completion results based on the AST. These are saved for merging. + // - completion results based on the AST. // - partial identifier and context. We need these for the index query. CompletionList Output; + auto RecorderOwner = llvm::make_unique<CompletionRecorder>(Opts, [&]() { + assert(Recorder && "Recorder is not set"); + Output = runWithSema(); + SPAN_ATTACH(Tracer, "sema_completion_kind", + getCompletionKindString(Recorder->CCContext.getKind())); + }); + + Recorder = RecorderOwner.get(); semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(), - SemaCCInput, [&] { - if (Recorder.CCSema) { - Output = runWithSema(); - SPAN_ATTACH( - Tracer, "sema_completion_kind", - getCompletionKindString(Recorder.CCContext.getKind())); - } else - log("Code complete: no Sema callback, 0 results"); - }); + SemaCCInput); SPAN_ATTACH(Tracer, "sema_results", NSema); SPAN_ATTACH(Tracer, "index_results", NIndex); @@ -883,7 +881,7 @@ private: // Sema data structures are torn down. It does all the real work. CompletionList runWithSema() { Filter = FuzzyMatcher( - Recorder.CCSema->getPreprocessor().getCodeCompletionFilter()); + Recorder->CCSema->getPreprocessor().getCodeCompletionFilter()); // Sema provides the needed context to query the index. // FIXME: in addition to querying for extra/overlapping symbols, we should // explicitly request symbols corresponding to Sema results. @@ -891,7 +889,7 @@ private: // We must copy index results to preserve them, but there are at most Limit. auto IndexResults = queryIndex(); // Merge Sema and Index results, score them, and pick the winners. - auto Top = mergeResults(Recorder.Results, IndexResults); + auto Top = mergeResults(Recorder->Results, IndexResults); // Convert the results to the desired LSP structs. CompletionList Output; for (auto &C : Top) @@ -901,7 +899,7 @@ private: } SymbolSlab queryIndex() { - if (!Opts.Index || !allowIndex(Recorder.CCContext.getKind())) + if (!Opts.Index || !allowIndex(Recorder->CCContext.getKind())) return SymbolSlab(); trace::Span Tracer("Query index"); SPAN_ATTACH(Tracer, "limit", Opts.Limit); @@ -912,8 +910,8 @@ private: if (Opts.Limit) Req.MaxCandidateCount = Opts.Limit; Req.Query = Filter->pattern(); - Req.Scopes = - getQueryScopes(Recorder.CCContext, Recorder.CCSema->getSourceManager()); + Req.Scopes = getQueryScopes(Recorder->CCContext, + Recorder->CCSema->getSourceManager()); log(llvm::formatv("Code complete: fuzzyFind(\"{0}\", scopes=[{1}])", Req.Query, llvm::join(Req.Scopes.begin(), Req.Scopes.end(), ","))); @@ -945,7 +943,7 @@ private: return nullptr; }; // Emit all Sema results, merging them with Index results if possible. - for (auto &SemaResult : Recorder.Results) + for (auto &SemaResult : Recorder->Results) addCandidate(Top, &SemaResult, CorrespondingIndexResult(SemaResult)); // Now emit any Index-only results. for (const auto &IndexResult : IndexResults) { @@ -962,7 +960,7 @@ private: CompletionCandidate C; C.SemaResult = SemaResult; C.IndexResult = IndexResult; - C.Name = IndexResult ? IndexResult->Name : Recorder.getName(*SemaResult); + C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult); CompletionItemScores Scores; if (auto FuzzyScore = Filter->match(C.Name)) @@ -986,7 +984,7 @@ private: const CompletionItemScores &Scores) { CodeCompletionString *SemaCCS = nullptr; if (auto *SR = Candidate.SemaResult) - SemaCCS = Recorder.codeCompletionString(*SR, Opts.IncludeBriefComments); + SemaCCS = Recorder->codeCompletionString(*SR, Opts.IncludeBriefComments); return Candidate.build(FileName, Scores, Opts, SemaCCS); } }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits