[YAML] Recover gracefully when deserializing invalid YAML input.

[tulloch]

Summary

1. See http://llvm.org/bugs/show_bug.cgi?id=16221 for a user's bug report.
2. We weren't checking for Document::getRoot() returning NULL. This happens

in the case where the input YAML/JSON object is malformed.

3. Document::getRoot() is passed to isa<NullNode>, which asserts on it's

argument being NULL.

4. To fix this, we check for this return value, and set the error code

accordingly. We also modify the calling code to check the return value
of input::setCurrentDocument().

The added unit tests hit the assert in trunk, while passing on this branch.

Additionally, the command that reproduces this assert in the bug report:

llvm-build/Debug+Asserts/bin/clang-format  -style="{BasedOnStyle:llvm}" tools/clang/tools/clang-format/ClangFormat.cpp

now outputs http://llvm-reviews.chandlerc.com/P55

[tulloch]

Removed braces around single-line conditional block.

[kledzik]

LGTM

include/llvm/Support/YAMLTraits.h

961

Wouldn't this whole body be simpler as:

if (yin.setCurrentDocument())
  yamlize(yin, docMap, true);
return yin;

[tulloch]

include/llvm/Support/YAMLTraits.h

961

Yes, that would be simpler. I'll update.

lib/Support/YAMLTraits.cpp

53

This call can potentially output errors without allowing us to set the DiagHandler.

unittests/Support/YAMLIOTest.cpp

1373–1374

This unfortunately outputs an error message into the unit test logs, since Document.begin() is called before we setDiagHandler.

We can convert it to

a) optionally pass DiagHandler in the constructor,
b) lazily initialize the DocIterator,
c) explicitly initialize Input, etc.
d) just ignore it.

Any thoughts on this?

[tulloch]

Cleaned up control flow somewhat.

[tulloch]

ping?

[alexfh]

Does this handle empty input well (http://llvm.org/PR15927)? If so, please add a test.

Plus a few comments inline.

unittests/Support/YAMLIOTest.cpp

76

nit: add a space after the comma.

1373–1374

Option a) seems like the best solution here.

[tulloch]

@alexfh, thanks for the comments (and the pointer to the other bug report). Will update the diff with your suggested changes.

unittests/Support/YAMLIOTest.cpp

76

Sure, good catch.

[tulloch]

Updated with reviewer comments, and added unit tests covering the empty string for the sequence/map with {required, optional} keys cases.

[tulloch]

Fixed spelling error in unit test naming.

[alexfh]

Thanks for the updated patch! It looks good overall. Please see a few comments inline.

include/llvm/Support/YAMLTraits.h

692

nit: Please add spaces around assignments.

I'd also rename Handler to DiagHandler and rephrase the comment:
"The DiagHandler can be specified to provide alternative error reporting."

702

I don't see any need in this method, as it's only used directly after constructor (which can be wrong, if the constructor call generates errors on its own). So I suggest removing it.

I only could find usages of this method in two files:
unittests/Support/YAMLIOTest.cpp
tools/clang/tools/extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

(the second one resides in a different branch of the repository, and can be updated separately after this patch gets in).

lib/Support/YAMLTraits.cpp

118

Is CurrentNode NULL only when the document is empty? Can it be NULL when parsing invalid documents?

150

nit: please put a period in the end of the comment.

unittests/Support/YAMLIOTest.cpp

1372

nit: in LLVM it's more common to write:
/*Ctxt=*/NULL,

[tulloch]

Updated with reviewer comments.

[tulloch]

lib/Support/YAMLTraits.cpp

118

It would be null in the case where the document is invalid, yes. In that case we would have set EC in setCurrentDocument(), and so wouldn't have reached this part of the method.

Would you like this comment to be clarified?

[alexfh]

Looks good with one change in a comment.

Do you commit access? If not, I can commit the patch for you (and make the change in the comment myself).

lib/Support/YAMLTraits.cpp

118

Yes, the comment needs to be clarified. Mostly by removing the description of _what_ we're doing, and leaving only _why_, e.g. "CurrentNode is null for empty documents, which is an error in the presence of required keys." or something similar. What do you think?

[tulloch]

@alexfh - thanks for reviewing the patch. I don't have commit access, and feel free to commit the patch (with whatever changes you feel like - the rewording you proposed is much clearer).

Thanks!

Andrew

lib/Support/YAMLTraits.cpp

118

I think your wording is much clearer, thanks!

[alexfh]

Committed revision 195016.

include/llvm/Support/YAMLTraits.h

 /// each yaml Node.  The extra layer is buffering.  The low level yaml
 /// parser only lets you look at each node once.  The buffering layer lets
 /// you search and interate multiple times.  This is necessary because
 /// the mapRequired() method calls may not be in the same order
 /// as the keys in the document.
 ///
 class Input : public IO {
 public:
-  // Construct a yaml Input object from a StringRef and optional user-data.
-  Input(StringRef InputContent, void *Ctxt=NULL);
+  // Construct a yaml Input object from a StringRef and optional
+  // user-data. The DiagHandler can also be set in the constructor,
+  // which means that it can be called from the parsing taking place
+  // in the construct of Input.
+  Input(StringRef InputContent,
+        void *Ctxt=NULL,
+        SourceMgr::DiagHandlerTy Handler=NULL,
+        void *HandlerCtxt=NULL);
   ~Input();
   
   // Check if there was an syntax or semantic error during parsing.
   llvm::error_code error();
 
   // To set alternate error reporting.
+  // Overrides the any handler set in the constructor of Input.
   void setDiagHandler(llvm::SourceMgr::DiagHandlerTy Handler, void *Ctxt = 0);
 
 private:
   virtual bool outputting();
   virtual void beginMapping();
   virtual void endMapping();
   virtual bool preflightKey(const char *, bool, bool, bool &, void *&);
   virtual void postflightKey(void *);
   virtual unsigned beginSequence();

 }
 
 // Define non-member operator>> so that Input can stream in a map as a document.
 template <typename T>
 inline
 typename llvm::enable_if_c<has_MappingTraits<T>::value,Input &>::type
 operator>>(Input &yin, T &docMap) {
   yin.setCurrentDocument();
   yamlize(yin, docMap, true);
   return yin;
 }
 
 // Define non-member operator>> so that Input can stream in a sequence as
 // a document.
 template <typename T>
 inline
 typename llvm::enable_if_c<has_SequenceTraits<T>::value,Input &>::type
 operator>>(Input &yin, T &docSeq) {
-  yin.setCurrentDocument();
+  if (yin.setCurrentDocument())
     yamlize(yin, docSeq, true);
   return yin;
 }
 
 // Provide better error message about types missing a trait specialization
 template <typename T>
 inline
 typename llvm::enable_if_c<missingTraits<T>::value,Input &>::type
 operator>>(Input &yin, T &docSeq) {

lib/Support/YAMLTraits.cpp

 void IO::setContext(void *Context) {
   Ctxt = Context;
 }
 
 //===----------------------------------------------------------------------===//
 //  Input
 //===----------------------------------------------------------------------===//
 
-Input::Input(StringRef InputContent, void *Ctxt)
+Input::Input(StringRef InputContent,
+             void *Ctxt,
+             SourceMgr::DiagHandlerTy Handler,
+             void *HandlerCtxt)
   : IO(Ctxt),
     Strm(new Stream(InputContent, SrcMgr)),
     CurrentNode(NULL) {
+  if (Handler)
+    setDiagHandler(Handler, HandlerCtxt);
   DocIterator = Strm->begin();
 }
 
 Input::~Input() {
 }
 
 error_code Input::error() {
   return EC;
 }
 
 void Input::setDiagHandler(SourceMgr::DiagHandlerTy Handler, void *Ctxt) {
   SrcMgr.setDiagHandler(Handler, Ctxt);
 }
 
 bool Input::outputting() {
   return false;
 }
 
 bool Input::setCurrentDocument() {
   if (DocIterator != Strm->end()) {
     Node *N = DocIterator->getRoot();
+    if (!N) {
+      assert(Strm->failed() && "Root is NULL iff parsing failed");
+      EC = make_error_code(errc::invalid_argument);
+      return false;
+    }
+
     if (isa<NullNode>(N)) {
       // Empty files are allowed and ignored
       ++DocIterator;
       return setCurrentDocument();
     }
     TopNode.reset(this->createHNodes(N));
     CurrentNode = TopNode.get();
     return true;
   }
   return false;
 }
 
 void Input::nextDocument() {
   ++DocIterator;
 }
 
 void Input::beginMapping() {
   if (EC)
     return;
-  MapHNode *MN = dyn_cast<MapHNode>(CurrentNode);
+  // CurrentNode can be null if the document is empty
+  MapHNode *MN = dyn_cast_or_null<MapHNode>(CurrentNode);
   if (MN) {
     MN->ValidKeys.clear();
   }
 }
 
 bool Input::preflightKey(const char *Key, bool Required, bool, bool &UseDefault,
                          void *&SaveInfo) {
   UseDefault = false;
   if (EC)
     return false;
+
+  // If CurrentNode is null (for example, the document is an empty
+  // string), then we check if the key is required, and if so, set the
+  // error code.  This somewhat convoluted logic is due to the fact
+  // that mappings without required keys can be validly deserialized
+  // from an empty YAML document, and so we must defer the decision of
+  // when to fail until we reach this point.
+  if (!CurrentNode) {
+    if (Required)
+      EC = make_error_code(errc::invalid_argument);
+    return false;
+  }
+
   MapHNode *MN = dyn_cast<MapHNode>(CurrentNode);
   if (!MN) {
     setError(CurrentNode, "not a mapping");
     return false;
   }
   MN->ValidKeys.push_back(Key);
   HNode *Value = MN->Mapping[Key];
   if (!Value) {

 
 void Input::postflightKey(void *saveInfo) {
   CurrentNode = reinterpret_cast<HNode *>(saveInfo);
 }
 
 void Input::endMapping() {
   if (EC)
     return;
-  MapHNode *MN = dyn_cast<MapHNode>(CurrentNode);
+  // CurrentNode can be null if the document is empty
+  MapHNode *MN = dyn_cast_or_null<MapHNode>(CurrentNode);
   if (!MN)
     return;
   for (MapHNode::NameToNode::iterator i = MN->Mapping.begin(),
        End = MN->Mapping.end(); i != End; ++i) {
     if (!MN->isValidKey(i->first())) {
       setError(i->second, Twine("unknown key '") + i->first() + "'");
       break;
     }
void Input::scalarString(StringRef &S) {
   if (ScalarHNode *SN = dyn_cast<ScalarHNode>(CurrentNode)) {
     S = SN->value();
   } else {
     setError(CurrentNode, "unexpected scalar");
   }
 }
 
 void Input::setError(HNode *hnode, const Twine &message) {
+  assert(hnode && "HNode must not be NULL");
   this->setError(hnode->_node, message);
 }
 
 void Input::setError(Node *node, const Twine &message) {
   Strm->printError(node, message);
   EC = make_error_code(errc::invalid_argument);
 }
 

unittests/Support/YAMLIOTest.cpp

 }
 
 
 //
 // Test the reading of a yaml mapping
 //
 TEST(YAMLIO, TestMapRead) {
   FooBar doc;
+  {
     Input yin("---\nfoo:  3\nbar:  5\n...\n");
     yin >> doc;
 
     EXPECT_FALSE(yin.error());
     EXPECT_EQ(doc.foo, 3);
-  EXPECT_EQ(doc.bar,5);
+    EXPECT_EQ(doc.bar, 5);
   }
 
+  {
+    Input yin("{foo: 3, bar: 5}");
+    yin >> doc;
+
+    EXPECT_FALSE(yin.error());
+    EXPECT_EQ(doc.foo, 3);
+    EXPECT_EQ(doc.bar, 5);
+  }
+}
 
 //
 // Test the reading of a yaml sequence of mappings
 //
 TEST(YAMLIO, TestSequenceMapRead) {
   FooBarSequence seq;
   Input yin("---\n - foo:  3\n   bar:  5\n - foo:  7\n   bar:  9\n...\n");
   yin >> seq;
Input yin("---\n"
             "- 0x12345567890ABCDEF0\n"
             "...\n");
   yin.setDiagHandler(suppressErrorMessages);
   yin >> seq;
 
   EXPECT_TRUE(yin.error());
 }
 
+TEST(YAMLIO, TestMalformedMapFailsGracefully) {
+  FooBar doc;
+  {
+    // We pass the suppressErrorMessages handler to handle the error
+    // message generated in the constructor of Input.
+    Input yin("{foo:3, bar: 5}", NULL /* ctxt */, suppressErrorMessages);
+    yin >> doc;
+    EXPECT_TRUE(yin.error());
+  }
+
+  {
+    Input yin("---\nfoo:3\nbar: 5\n...\n");
+    yin.setDiagHandler(suppressErrorMessages);
+    yin >> doc;
+    EXPECT_TRUE(yin.error());
+  }
+}
+
 struct OptionalTest {
   std::vector<int> Numbers;
 };
 
 struct OptionalTestSeq {
   std::vector<OptionalTest> Tests;
 };
 
TEST(YAMLIO, SequenceElideTest) {
   EXPECT_TRUE(Seq2.Tests[1].Numbers.empty());
 
   EXPECT_EQ(1, Seq2.Tests[2].Numbers[0]);
   EXPECT_EQ(2, Seq2.Tests[2].Numbers[1]);
   EXPECT_EQ(3, Seq2.Tests[2].Numbers[2]);
 
   EXPECT_TRUE(Seq2.Tests[3].Numbers.empty());
 }
+
+TEST(YAMLIO, TestEmptyStringFailsForMapWithRequiredFields) {
+  FooBar doc;
+  Input yin("");
+  yin >> doc;
+  EXPECT_TRUE(yin.error());
+}
+
+TEST(YAMLIO, TestEmptyStringSucceedsForMapWithOptionalFields) {
+  OptionalTest doc;
+  Input yin("");
+  yin >> doc;
+  EXPECT_FALSE(yin.error());
+}
+
+TEST(YAMLIO, TestEmptyStringSucceedsForSequence) {
+  std::vector<uint8_t> seq;
+  Input yin("");
+  yin.setDiagHandler(suppressErrorMessages);
+  yin >> seq;
+
+  EXPECT_FALSE(yin.error());
+  EXPECT_TRUE(seq.empty());
+}