Skip to content

Fix stack overflow with CData edge case#9

Open
edwardsnjd wants to merge 1 commit into
jinjor:masterfrom
edwardsnjd:master
Open

Fix stack overflow with CData edge case#9
edwardsnjd wants to merge 1 commit into
jinjor:masterfrom
edwardsnjd:master

Conversation

@edwardsnjd

Copy link
Copy Markdown

Thanks for this great library!

I hit an issue when parsing CData content ending with a square bracket, ]:

<a>
  <![CDATA[ note extra sq bracket here ->]]]>
</a>

I added some tests for this case and had a go at making them all pass again. My understanding of the spec is that nothing is parsed inside the CData content so we can safely chomp until the end token: https://www.w3.org/TR/REC-xml/#sec-cdata-sect

I'm an elm newcomer and didn't fully understand the existing implementation, so would appreciate any input.

When a CData block content ends with one or more "]" the parser fails with a
stack overflow.  This commit adds some tests for edge cases and changes the
implementation to pass.
Comment thread tests/XmlParserTest.elm
, test "cdata 8" <| expectSucceed "<a>a<![CDATA[b]]>c</a>" (Element "a" [] [ Text "abc" ])
, test "cdata 9" <| expectSucceed "<a><![CDATA[a[b]c]]></a>" (Element "a" [] [ Text "a[b]c" ])
, test "cdata 10" <| expectSucceed "<a><![CDATA[[b]c]]></a>" (Element "a" [] [ Text "[b]c" ])
, test "cdata 11" <| expectSucceed "<a><![CDATA[a[b]]]></a>" (Element "a" [] [ Text "a[b]" ])

@edwardsnjd edwardsnjd Aug 28, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case that was previously failing due to the trailing ].

Comment thread tests/XmlParserTest.elm
, test "cdata 10" <| expectSucceed "<a><![CDATA[[b]c]]></a>" (Element "a" [] [ Text "[b]c" ])
, test "cdata 11" <| expectSucceed "<a><![CDATA[a[b]]]></a>" (Element "a" [] [ Text "a[b]" ])
, test "cdata 12" <| expectSucceed "<a><![CDATA[a[[b]]c]]></a>" (Element "a" [] [ Text "a[[b]]c" ])
, test "cdata 13" <| expectSucceed "<a><![CDATA[ab<![CDATA[cd]]></a>" (Element "a" [] [ Text "ab<![CDATA[cd" ])

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests 13 and 14 are overkill but I wanted to prove that nested CData blocks are handled correctly.

Comment thread tests/XmlParserTest.elm
, test "cdata fail 3" <| expectFail "<a><![CDATA[]]</a>"
, test "cdata fail 4" <| expectFail "<a><![CDATA[abc</a>"
, test "cdata fail 5" <| expectFail "<a>abc<![CDATA[</a>"
, test "cdata fail 6" <| expectFail "<a>abc<![CDATA[def</a>"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, these are probably overkill but prove that invalid CData blocks do fail.

@edwardsnjd
edwardsnjd requested a review from jinjor January 14, 2020 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant