Closed Bug 362547 Opened 15 years ago Closed 15 years ago

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


(Core :: XML, defect)

Not set





(Reporter: WeirdAl, Assigned: WeirdAl)


(Keywords: crash, fixed1.8.1.2, testcase)


(5 files)

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.
Attached file gdb stack trace
Shoot, I thought we got all of these.
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.
This crash is not exposed in Firefox 2, because the feed parser does not attach an nsILexicalHandler.
Target Milestone: --- → mozilla1.9alpha
We should get this on the branch for in case extensions use it... but I can't set a relevant flag.
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).
Flags: blocking1.8.1.1?
Too late for this, especially if vanilla FF2 is unaffected. Moving nom to the branch flag rather than the specific release.
Flags: blocking1.8.1.1? → wanted1.8.1.x+
Attached patch patch, v1Splinter Review
Assignee: xml → ajvincent
Attachment #247238 - Flags: review?(sayrer)
Attachment #247238 - Flags: review?(sayrer) → review+
Attachment #247238 - Flags: superreview?(bzbarsky)
Comment on attachment 247238 [details] [diff] [review]
patch, v1

sr=bzbarsky; try to add some tests for this?
Attachment #247238 - Flags: superreview?(bzbarsky) → superreview+
Checking in nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.13; previous revision: 1.12
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #247238 - Flags: approval1.8.1.2?
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?
Flags: in-testsuite?
no, we should have a test/ directory in parser/xml containing SAX tests.
Attachment #247752 - Flags: review?(sayrer)
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 on attachment 247752 [details] [diff] [review]
xpcshell unit test

This is good to start.
Attachment #247752 - Flags: review?(sayrer) → review+
Checking in;
/cvsroot/mozilla/parser/xml/,v  <--
new revision: 1.2; previous revision: 1.1
RCS file: /cvsroot/mozilla/parser/xml/test/,v
Checking in test/;
/cvsroot/mozilla/parser/xml/test/,v  <--
initial revision: 1.1
RCS file: /cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v
Checking in test/unit/test_parser.js;
/cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v  <--  test_parser.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
Comment on attachment 247238 [details] [diff] [review]
patch, v1

Approved for 1.8 branch, a=jay for drivers.
Attachment #247238 - Flags: approval1.8.1.2? → approval1.8.1.2+
We're coming up on the code freeze, need a check-in on this if it's going to make
Whiteboard: [at risk] need check-in
Whiteboard: [at risk] need check-in → [at risk][checkin needed]
Checked in on branch.
Whiteboard: [at risk][checkin needed]
Keywords: fixed1.8.1.2
You need to log in before you can comment on or make changes to this bug.