362547 - Crash in SAXReader with doctype missing public, system ID's

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Alex Vincent [:WeirdAl]]

In trying to parse a XML document with:

<!DOCTYPE bindings>

I crashed in nsCharTraits<unsigned short>::length because (s=0x0).
nsSAXXMLReader::HandleStartDTD(), with a lexical handler set, joyfully calls for nsDependentString(nsnull), and kaboom.

Stack, testcase coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: gdb stack trace

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: testcase, part 1 of 2 (XML document)

Comment 3

[Robert Sayre]

Shoot, I thought we got all of these.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: testcase, part 2 of 2 (xpcshell script)

Steps to reproduce:
(1) Save the xpcshell script locally.
(2) gdb xpcshell (you need the debugger to get the stack trace) and run
(3) load the script into xpcshell.
(4) runTest();
(5) Wait about ten seconds; the script does do a fetch of the XML document from Bugzilla.

Comment 5

[Robert Sayre]

This crash is not exposed in Firefox 2, because the feed parser does not attach an nsILexicalHandler.

Comment 6

[Robert Sayre]

We should get this on the branch for 2.0.0.2 in case extensions use it... but I can't set a relevant flag.

Comment 7

[Robert Sayre]

requesting blocking to get opinion from drivers. I don't think it should actually block, but it would be a crasher if an extension hits it (I don't know of any that use LexicalHandler, other than Alex's).

Comment 8

[Daniel Veditz [:dveditz]]

Too late for this, especially if vanilla FF2 is unaffected. Moving nom to the branch flag rather than the specific release.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 247238 [details] [diff] [review]
patch, v1

sr=bzbarsky; try to add some tests for this?

Comment 11

[Robert Sayre]

Checking in nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.13; previous revision: 1.12
done

Comment 12

[Alex Vincent [:WeirdAl]]

per bz's request, setting in-testsuite? flag.  If we create a DOMParser instead of the XMLHttpRequest in the xpcshell script, it should be possible to make this a single-file testcase (JavaScript, xpcshell).  Isn't that what our "make check" system likes?

Comment 13

[Robert Sayre]

no, we should have a test/ directory in parser/xml containing SAX tests.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: xpcshell unit test

Comment 15

[Alex Vincent [:WeirdAl]]

Comment on attachment 247752 [details] [diff] [review]
xpcshell unit test

>+  if (parseErrorLog.length > 0) {
>+    dump(parseErrorLog.join("\n"));
>+  }
>+  do_check_true_with_dump(parseErrorLog.length == 0, parseErrorLog);

I don't know how I let the first three lines of that slip through. :(

Comment 16

[Robert Sayre]

Comment on attachment 247752 [details] [diff] [review]
xpcshell unit test

This is good to start.

Comment 17

[Robert Sayre]

Checking in Makefile.in;
/cvsroot/mozilla/parser/xml/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/parser/xml/test/Makefile.in,v
done
Checking in test/Makefile.in;
/cvsroot/mozilla/parser/xml/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v
done
Checking in test/unit/test_parser.js;
/cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v  <--  test_parser.js
initial revision: 1.1
done

Comment 18

[Jay Patel [:jay]]

Comment on attachment 247238 [details] [diff] [review]
patch, v1

Approved for 1.8 branch, a=jay for drivers.

Comment 19

[Daniel Veditz [:dveditz]]

We're coming up on the code freeze, need a check-in on this if it's going to make 2.0.0.2

Comment 20

[Jeff Walden [:Waldo]]

Checked in on branch.