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

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Feb 19 01:31:26 2018
New Revision: 325476

URL: http://llvm.org/viewvc/llvm-project?rev=325476&view=rev
Log:
[clangd] Fix use-after-free in SymbolYAML: strings are owned by yaml::Input!

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.

(Also fix an unused-var that sneaked in)

Reviewers: ioeric

Subscribers: klimek, ilya-biryukov, jkorous-apple, cfe-commits

Differential Revision: https://reviews.llvm.org/D43381

Modified:
    clang-tools-extra/trunk/clangd/XRefs.cpp
    clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
    clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
    clang-tools-extra/trunk/clangd/index/SymbolYAML.h

Modified: clang-tools-extra/trunk/clangd/XRefs.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/XRefs.cpp?rev=325476&r1=325475&r2=325476&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/XRefs.cpp (original)
+++ clang-tools-extra/trunk/clangd/XRefs.cpp Mon Feb 19 01:31:26 2018
@@ -360,9 +360,8 @@ static std::string NamedDeclQualifiedNam
 static llvm::Optional<std::string> getScopeName(const Decl *D) {
   const DeclContext *DC = D->getDeclContext();
 
-  if (const TranslationUnitDecl *TUD = dyn_cast<TranslationUnitDecl>(DC))
+  if (isa<TranslationUnitDecl>(DC))
     return std::string("global namespace");
-
   if (const TypeDecl *TD = dyn_cast<TypeDecl>(DC))
     return TypeDeclToString(TD);
   else if (const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC))

Modified: clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp?rev=325476&r1=325475&r2=325476&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp (original)
+++ clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp Mon Feb 19 01:31:26 2018
@@ -20,7 +20,6 @@
 #include "index/SymbolYAML.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
-#include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/CommonOptionsParser.h"
@@ -31,6 +30,7 @@
 #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;
@@ -117,7 +117,8 @@ SymbolSlab mergeSymbols(tooling::ToolRes
   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))

Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=325476&r1=325475&r2=325476&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Mon Feb 19 01:31:26 2018
@@ -12,7 +12,6 @@
 #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)
@@ -176,11 +175,11 @@ SymbolSlab SymbolsFromYAML(llvm::StringR
   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;
 }
 

Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.h?rev=325476&r1=325475&r2=325476&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.h (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.h Mon Feb 19 01:31:26 2018
@@ -20,6 +20,7 @@
 
 #include "Index.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
@@ -28,9 +29,10 @@ 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.