Fix CData parsing in CFDE_XMLSyntaxParser.

Fix CData parsing in CFDE_XMLSyntaxParser.

This CL splits the handling of CData sections out to an individual phase
of the parser. This fixes the issue with the CData parser getting confused
by < characters inside the data section.

BUG=pdfium:90
Committed: https://pdfium.googlesource.com/pdfium/+/11ac93cfdb9f4f25eee2ba60b947f992ab40ec54

[dsinclair, 2016-03-30 03:48:29]

dsinclair@chromium.org changed reviewers:
+ tsepez@chromium.org

[dsinclair, 2016-03-30 03:48:29]

PTAL.

This works, but it feels sketchy, not sure why.

[Tom Sepez, 2016-03-30 16:45:16]

On 2016/03/30 03:48:29, dsinclair wrote:
> PTAL.
> 
> This works, but it feels sketchy, not sure why.

Can we write a test case first?

[Tom Sepez, 2016-03-30 16:45:45]

On 2016/03/30 16:45:16, Tom Sepez wrote:
> On 2016/03/30 03:48:29, dsinclair wrote:
> > PTAL.
> > 
> > This works, but it feels sketchy, not sure why.
> 
> Can we write a test case first?
Nevermind, I can't read.

[Tom Sepez, 2016-03-30 16:55:30]

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
File xfa/fde/xml/fde_xml_imp_unittest.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:13: "  <![CDATA[\n"
I'd like to see some variations on this as in
<!>
<![>
<![CDATA>
<![CDATA[]>
etc.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:15: "      app.alert(\"Tclams\");\n"
Since were'e in cdata, we should test having an embedded </script> tag in the
section which should be ignored.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:26: size_t len = FXSYS_wcslen(input) * 2;
Needs to be sizeof(FX_WCHAR) (2 vs. 4 depending on platform).

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:27: std::unique_ptr<IFX_Stream> stream =
std::unique_ptr<IFX_Stream>(
Just std::unique_ptr<IFX_Stream> stream(FX_Stream::CreateStream((uint8_t*)input,
len, 0));
also reinterpret_cast<uint8_t*>

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:30: parser.Init(stream.get(), 256);
why 256?

https://codereview.chromium.org/1842633004/diff/1/xfa/fgas/crt/fgas_stream.cpp
File xfa/fgas/crt/fgas_stream.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fgas/crt/fgas_stream.cp...
xfa/fgas/crt/fgas_stream.cpp:873: while (*pSrc != L'\0' && iCount < iLen) {
nit: just while (*pSrc && iCount < iLen) {

[Tom Sepez, 2016-03-30 17:04:41]

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp
File xfa/fde/xml/fde_xml_imp.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1786: if (ch == '-') {
This is wrong, too, needs -- to enter comment, right?

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1788: } else if (ch == '[') {
Maybe look for the whole "[CDATA[" here, otherwise, its skipdelcnode?

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1797: if (ch == ']' && (m_pStart + 2 < m_pEnd) &&
*(m_pStart + 1) == ']' &&
maybe its easier to just look for "]]>" via string comparision.

[dsinclair, 2016-03-30 17:32:13]

I've added more tests, they don't currently pass, but can you verify that the
results I'm expecting are what you'd expect?

Mostly around, what do we output for invalid CData and comment sections.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
File xfa/fde/xml/fde_xml_imp_unittest.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:13: "  <![CDATA[\n"
On 2016/03/30 16:55:30, Tom Sepez wrote:
> I'd like to see some variations on this as in
> <!>
> <![>
> <![CDATA>
> <![CDATA[]>
> etc.


Added a bunch of tests. Any more? (And a bunch of todo's where the result seemed
weird.)

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:15: "      app.alert(\"Tclams\");\n"
On 2016/03/30 16:55:30, Tom Sepez wrote:
> Since were'e in cdata, we should test having an embedded </script> tag in the
> section which should be ignored.

Done.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:26: size_t len = FXSYS_wcslen(input) * 2;
On 2016/03/30 16:55:30, Tom Sepez wrote:
> Needs to be sizeof(FX_WCHAR) (2 vs. 4 depending on platform).

Done.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:27: std::unique_ptr<IFX_Stream> stream =
std::unique_ptr<IFX_Stream>(
On 2016/03/30 16:55:30, Tom Sepez wrote:
> Just std::unique_ptr<IFX_Stream>
stream(FX_Stream::CreateStream((uint8_t*)input,
> len, 0));
> also reinterpret_cast<uint8_t*>

Done.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp_uni...
xfa/fde/xml/fde_xml_imp_unittest.cpp:30: parser.Init(stream.get(), 256);
On 2016/03/30 16:55:30, Tom Sepez wrote:
> why 256?

No particular reason. Has to be > 0, and at 256 it's longer then my string
length so I think the parser should consume it in one go.

https://codereview.chromium.org/1842633004/diff/1/xfa/fgas/crt/fgas_stream.cpp
File xfa/fgas/crt/fgas_stream.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fgas/crt/fgas_stream.cp...
xfa/fgas/crt/fgas_stream.cpp:873: while (*pSrc != L'\0' && iCount < iLen) {
On 2016/03/30 16:55:30, Tom Sepez wrote:
> nit: just while (*pSrc && iCount < iLen) {

Done.

[dsinclair, 2016-03-30 19:18:12]

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp
File xfa/fde/xml/fde_xml_imp.cpp (right):

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1786: if (ch == '-') {
On 2016/03/30 17:04:41, Tom Sepez wrote:
> This is wrong, too, needs -- to enter comment, right?

Done.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1788: } else if (ch == '[') {
On 2016/03/30 17:04:41, Tom Sepez wrote:
> Maybe look for the whole "[CDATA[" here, otherwise, its skipdelcnode?

Done.

https://codereview.chromium.org/1842633004/diff/1/xfa/fde/xml/fde_xml_imp.cpp...
xfa/fde/xml/fde_xml_imp.cpp:1797: if (ch == ']' && (m_pStart + 2 < m_pEnd) &&
*(m_pStart + 1) == ']' &&
On 2016/03/30 17:04:41, Tom Sepez wrote:
> maybe its easier to just look for "]]>" via string comparision.

Done.

[Tom Sepez, 2016-03-30 20:07:12]

If I wanted to be picky, there's these, which I believe are usually treated as
complete comments by the  more forgiving parsers:
<!-->
<!--->
<!---->

and this, which is treated as a run-on:
<!-- comment ->

But we should be able to find someone to tell us for sure.  Until then LGTM.

https://codereview.chromium.org/1842633004/diff/40001/xfa/fde/xml/fde_xml_imp...
File xfa/fde/xml/fde_xml_imp_unittest.cpp (right):

https://codereview.chromium.org/1842633004/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp_unittest.cpp:66: TEST(CFDE_XMLSyntaxParser,
CData_with_inner_script) {
nit: prefer CDataWithInnerScript

[Tom Sepez, 2016-03-30 20:35:33]

Opening XML file with chrome shows that
<!-->
<!--->
are unterminated comments, too.

[dsinclair, 2016-03-31 13:40:37]

PTAL.

I added the tests for the unterminated comments, and fixed the comment parsing
to correctly pass the test. This changes the check for completion of comment to
require 2 '--'s and changes to a string comparison.

I also removed some extraneous code since we added the CData parsing.

https://codereview.chromium.org/1842633004/diff/40001/xfa/fde/xml/fde_xml_imp...
File xfa/fde/xml/fde_xml_imp_unittest.cpp (right):

https://codereview.chromium.org/1842633004/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp_unittest.cpp:66: TEST(CFDE_XMLSyntaxParser,
CData_with_inner_script) {
On 2016/03/30 20:07:12, Tom Sepez wrote:
> nit: prefer CDataWithInnerScript

Done.

[Tom Sepez, 2016-03-31 16:12:23]

LGTM.


In reading this, I'm mentally translating "FDE_XMLSYNTAXMODE_*" into
FDE_XMLSYNTAXSTATE_*, and  FDE_XMLSYNTAXSTATUS_* into FDE_XMLSYNTAXRESULT_* with
the locals/member names changed to match. Then it makes sense to my tiny brain. 
Maybe a followon?

[dsinclair, 2016-03-31 16:44:57]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-03-31 16:45:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842633004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842633004/80001

[commit-bot: I haz the power, 2016-03-31 16:45:25]

Description was changed from

==========
Fix CData parsing in CFDE_XMLSyntaxParser.

This CL splits the handling of CData sections out to an individual phase
of the parser. This fixes the issue with the CData parser getting confused
by < characters inside the data section.

BUG=pdfium:90
==========

to

==========
Fix CData parsing in CFDE_XMLSyntaxParser.

This CL splits the handling of CData sections out to an individual phase
of the parser. This fixes the issue with the CData parser getting confused
by < characters inside the data section.

BUG=pdfium:90

Committed:
https://pdfium.googlesource.com/pdfium/+/11ac93cfdb9f4f25eee2ba60b947f992ab40...
==========

[commit-bot: I haz the power, 2016-03-31 16:45:26]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/11ac93cfdb9f4f25eee2ba60b947f992ab40...