[clangd] Fix use-after-free in SymbolYAML: strings are owned by yaml::Input!

[sammccall]

Summary

There are a few implementation options here - alternatives are either both
awkward and inefficient, or really inefficient.
This is at least potentially a hot path when used as a reducer for common
symbols.

[ioeric]

Thanks a lot for the fix!

Suggest a test case which reproduced the bug:

// comment with 'quote'
void f() {}

runSymbolCollector -> SymbolToYAML -> SymbolFromYAML -> check Documentation

clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

 // production code.
 //
 //===---------------------------------------------------------------------===//
 
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
-#include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/ThreadPool.h"
+#include "llvm/Support/YAMLTraits.h"
 
 using namespace llvm;
 using namespace clang::tooling;
 using clang::clangd::SymbolSlab;
 
 namespace clang {
 namespace clangd {
 namespace {

 
 // Combine occurrences of the same symbol across translation units.
 SymbolSlab mergeSymbols(tooling::ToolResults *Results) {
   SymbolSlab::Builder UniqueSymbols;
   llvm::BumpPtrAllocator Arena;
   Symbol::Details Scratch;
   Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) {
     Arena.Reset();
-    auto Sym = clang::clangd::SymbolFromYAML(Value, Arena);
+    llvm::yaml::Input Yin(Value, &Arena);
+    auto Sym = clang::clangd::SymbolFromYAML(Yin, Arena);
     clang::clangd::SymbolID ID;
     Key >> ID;
     if (const auto *Existing = UniqueSymbols.find(ID))
       UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
     else
       UniqueSymbols.insert(Sym);
   });
   return std::move(UniqueSymbols).build();

clangd/index/SymbolYAML.h

 //
 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_FROM_YAML_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_FROM_YAML_H
 
 #include "Index.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 
 // Read symbols from a YAML-format string.
 SymbolSlab SymbolsFromYAML(llvm::StringRef YAMLContent);
 
-// Read one symbol from a YAML-format string, backed by the arena.
-Symbol SymbolFromYAML(llvm::StringRef YAMLContent,
-                      llvm::BumpPtrAllocator &Arena);
+// Read one symbol from a YAML-stream.
+// The arena must be the Input's context! (i.e. yaml::Input Input(Text, &Arena))
+// The returned symbol is backed by both Input and Arena.
+Symbol SymbolFromYAML(llvm::yaml::Input &Input, llvm::BumpPtrAllocator &Arena);
 
 // Convert a single symbol to YAML-format string.
 // The YAML result is safe to concatenate.
 std::string SymbolToYAML(Symbol Sym);
 
 // Convert symbols to a YAML-format string.
 // The YAML result is safe to concatenate if you have multiple symbol slabs.
 void SymbolsToYAML(const SymbolSlab& Symbols, llvm::raw_ostream &OS);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_FROM_YAML_H

clangd/index/SymbolYAML.cpp

 //===--- SymbolYAML.cpp ------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolYAML.h"
 #include "Index.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(clang::clangd::Symbol)
 
 namespace llvm {
 namespace yaml {
 
 using clang::clangd::Symbol;
SymbolSlab SymbolsFromYAML(llvm::StringRef YAMLContent) {
   Yin >> S;
 
   SymbolSlab::Builder Syms;
   for (auto &Sym : S)
     Syms.insert(Sym);
   return std::move(Syms).build();
 }
 
-Symbol SymbolFromYAML(llvm::StringRef YAMLContent,
-                      llvm::BumpPtrAllocator &Arena) {
-  llvm::yaml::Input Yin(YAMLContent, &Arena);
+Symbol SymbolFromYAML(llvm::yaml::Input &Input, llvm::BumpPtrAllocator &Arena) {
+  // We could grab Arena out of Input, but it'd be a huge hazard for callers.
+  assert(Input.getContext() == &Arena);
   Symbol S;
-  Yin >> S;
+  Input >> S;
   return S;
 }
 
 void SymbolsToYAML(const SymbolSlab& Symbols, llvm::raw_ostream &OS) {
   llvm::yaml::Output Yout(OS);
   for (Symbol S : Symbols) // copy: Yout<< requires mutability.
     Yout << S;
 }