[PATCH] Detect malformed yaml sequence in yaml::Input::beginSequence()

[Justin Bogner]

William Fisher <william.w.fisher at gmail.com> writes:
> When reading a yaml::SequenceTraits object, YAMLIO does report an
> error if the yaml item is not a sequence. Instead, YAMLIO reads an
> empty sequence. For example:
>
> ---
> seq:
>     foo: 1
>     bar: 2
> ...
>
> If `seq` is a SequenceTraits object, then reading the above yaml will
> yield `seq` as an empty sequence.  The enclosed patch reports an error
> for the above mapping ("not a sequence")

LGTM with a couple of nits.

> Null and empty nodes are still interpreted as empty sequences:
>
> ---
> seq1:               # Empty node okay
> ---
> seq2:  null       # null okay
> ...
>
> Index: lib/Support/YAMLTraits.cpp
> ===================================================================
> --- lib/Support/YAMLTraits.cpp	(revision 224940)
> +++ lib/Support/YAMLTraits.cpp	(working copy)
> @@ -170,7 +170,16 @@
>  unsigned Input::beginSequence() {
>    if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
>      return SQ->Entries.size();
> +  } else if (dyn_cast<EmptyHNode>(CurrentNode)) {

This should be isa<EmptyHNode>(CurrentNode), since we don't use the
result.

> +    return 0;
>    }
> +  // Treat case where there's a scalar "null" value as an empty sequence.
> +  if (ScalarHNode *SN = dyn_cast<ScalarHNode>(CurrentNode)) {

Seems odd to use "else if" above and just "if" here. Better to be
consistent.

> +    if (isNull(SN->value()))
> +      return 0;
> +  }
> +  // Any other type of HNode is an error.
> +  setError(CurrentNode, "not a sequence");
>    return 0;
>  }
>  
> @@ -193,10 +202,7 @@
>  }
>  
>  unsigned Input::beginFlowSequence() {
> -  if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode)) {
> -    return SQ->Entries.size();
> -  }
> -  return 0;
> +  return beginSequence();
>  }
>  
>  bool Input::preflightFlowElement(unsigned index, void *&SaveInfo) {
> Index: unittests/Support/YAMLIOTest.cpp
> ===================================================================
> --- unittests/Support/YAMLIOTest.cpp	(revision 224940)
> +++ unittests/Support/YAMLIOTest.cpp	(working copy)
> @@ -46,6 +46,9 @@
>  
>  LLVM_YAML_IS_SEQUENCE_VECTOR(FooBar)
>  
> +struct FooBarContainer {
> +  FooBarSequence fbs;
> +};
>  
>  namespace llvm {
>  namespace yaml {
> @@ -56,6 +59,13 @@
>        io.mapRequired("bar",    fb.bar);
>      }
>    };
> +
> +  template <>
> +  struct MappingTraits<FooBarContainer> {
> +    static void mapping(IO &io, FooBarContainer& fb) {
> +      io.mapRequired("fbs",    fb.fbs);
> +    }
> +  };
>  }
>  }
>  
> @@ -109,8 +119,85 @@
>    EXPECT_EQ(map2.bar, 9);
>  }
>  
> +//
> +// Test the reading of a map containing a yaml sequence of mappings
> +//
> +TEST(YAMLIO, TestContainerSequenceMapRead) {
> +  {
> +    FooBarContainer cont;
> +    Input yin2("---\nfbs:\n - foo: 3\n   bar: 5\n - foo: 7\n   bar: 9\n...\n");
> +    yin2 >> cont;
> +  
> +    EXPECT_FALSE(yin2.error());
> +    EXPECT_EQ(cont.fbs.size(), 2UL);
> +    EXPECT_EQ(cont.fbs[0].foo, 3);
> +    EXPECT_EQ(cont.fbs[0].bar, 5);
> +    EXPECT_EQ(cont.fbs[1].foo, 7);
> +    EXPECT_EQ(cont.fbs[1].bar, 9);
> +  }
>  
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs:\n...\n");
> +    yin >> cont;
> +    // Okay: Empty node represents an empty array.
> +    EXPECT_FALSE(yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs: !!null null\n...\n");
> +    yin >> cont;
> +    // Okay: null represents an empty array.
> +    EXPECT_FALSE(yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs: ~\n...\n");
> +    yin >> cont;
> +    // Okay: null represents an empty array.
> +    EXPECT_FALSE(yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs: null\n...\n");
> +    yin >> cont;
> +    // Okay: null represents an empty array.
> +    EXPECT_FALSE(yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +}
> +
>  //
> +// Test the reading of a map containing a malformed yaml sequence
> +// 
> +TEST(YAMLIO, TestMalformedContainerSequenceMapRead) {
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs:\n   foo: 3\n   bar: 5\n...\n", nullptr, 
> +              suppressErrorMessages);
> +    yin >> cont;
> +    // Error: fbs is not a sequence.
> +    EXPECT_TRUE(!!yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +
> +  {
> +    FooBarContainer cont;
> +    Input yin("---\nfbs: 'scalar'\n...\n", nullptr, suppressErrorMessages);
> +    yin >> cont;
> +    // This should be an error.
> +    EXPECT_TRUE(!!yin.error());
> +    EXPECT_EQ(cont.fbs.size(), 0UL);
> +  }
> +}
> +
> +//
>  // Test writing then reading back a sequence of mappings
>  //
>  TEST(YAMLIO, TestSequenceMapWriteAndRead) {
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits