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

[William Fisher]

Thanks. I've fixed the suggested lines. Here is the new patch.



On Sat, Jan 17, 2015 at 2:15 PM, Justin Bogner <mail at justinbogner.com> wrote:
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: yaml-not-a-seq2.patch
Type: application/octet-stream
Size: 3822 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150118/7ec29d3c/attachment.obj>