[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

960

Wouldn't this whole body be simpler as:

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

[tulloch]

include/llvm/Support/YAMLTraits.h

960

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

lib/Support/YAMLTraits.cpp

47

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

unittests/Support/YAMLIOTest.cpp

1317–1318

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.

1317–1318

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."

694

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

112

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

131

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

unittests/Support/YAMLIOTest.cpp

1316

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

[tulloch]

Updated with reviewer comments.

[tulloch]

lib/Support/YAMLTraits.cpp

112

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

112

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

112

I think your wording is much clearer, thanks!

[alexfh]

Committed revision 195016.

include/llvm/Support/YAMLTraits.h

 class Input : public IO {
 public:
   // Construct a yaml Input object from a StringRef and optional user-data.
   Input(StringRef InputContent, void *Ctxt=NULL);
   ~Input();
   
   // Check if there was an syntax or semantic error during parsing.
   llvm::error_code error();
 
   // To set alternate error reporting.
   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();
operator>>(Input &yin, T &docList) {
   return yin;
 }
 
 // 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();
+  if (!yin.setCurrentDocument()) {
+    return yin;
+  }
   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()) {
+    return yin;
+  }
   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

lib/Support/YAMLTraits.cpp

 //===----------------------------------------------------------------------===//
 //  Input
 //===----------------------------------------------------------------------===//
 
 Input::Input(StringRef InputContent, void *Ctxt) 
   : IO(Ctxt), 
     Strm(new Stream(InputContent, SrcMgr)),
     CurrentNode(NULL) {
   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;
if (EC)
     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) {
     if (Required)
       setError(CurrentNode, Twine("missing required key '") + Key + "'");
     else
       UseDefault = true;
     return false;
   }
   SaveInfo = CurrentNode;
   CurrentNode = Value;
   return true;
 }
 
 void Input::postflightKey(void *saveInfo) {
   CurrentNode = reinterpret_cast<HNode *>(saveInfo);
 }
 
 void Input::endMapping() {
   if (EC)
     return;
   MapHNode *MN = dyn_cast<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;
     }

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);
   }
 
+  {
+    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 error handling with a malformed object
+//
+TEST(YAMLIO, TestMalformedMap) {
+  FooBar doc;
+  {
+    Input yin("{foo:3, bar: 5}");
+    yin.setDiagHandler(suppressErrorMessages);
+    yin >> doc;
+    EXPECT_TRUE(yin.error());
+  }
+
+  {
+    Input yin("---\nfoo:3\nbar: 5\n...\n");
+    yin.setDiagHandler(suppressErrorMessages);
+    yin >> doc;
+    EXPECT_TRUE(yin.error());
+  }
+}