324422 - Crash when creating a new E4X XML object using a large string

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Doug Wright]

100% reproducible crasher

var someXML = new XML("a large string")

See testcase

TB14315434H

Comment 1

[Doug Wright]

Bugzilla won't let me attach the file - so I've put it on my webspace

http://www.dougweb.org/bugzilla/324422.html

Comment 2

[Bob Clary [:bc] (inactive)]

I don't get the same stack as in the talkback report. In both 1.5 and trunk on winxp I see


            /* Store kid in xml right away, to protect it from GC. */
->            XMLARRAY_SET_MEMBER(&xml->xml_kids, i, kid);
            kid->parent = xml;
            ++i;

  
	i	0x00000000
-	kid	0x0108fcd0
+	object	0x00000000
	domnode	0x00000000
+	parent	0x00000000
+	name	0x00000000
	xml_class	0x0004
	xml_flags	0x0000
+	u	{...}
+	&xml	0x0012dbbc


ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x04c998c0, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1520 + 3 bytes
ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x04c99848, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1508 + 21 bytes
ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x04c942f8, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1508 + 21 bytes
ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x04c8a148, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1508 + 21 bytes
ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x03ea5c50, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1508 + 21 bytes
ParseNodeToXML(JSContext * 0x03c50818, JSParseNode * 0x034edda0, JSXMLArray * 0x0012e3a4, unsigned int 0x0000001f) line 1508 + 21 bytes
ParseXMLSource(JSContext * 0x03c50818, JSString * 0x014866a8) line 1953 + 21 bytes
ToXML(JSContext * 0x03c50818, long 0x014866ac) line 2048 + 13 bytes
XML(JSContext * 0x03c50818, JSObject * 0x0333bc40, unsigned int 0x00000001, long * 0x0612672c, long * 0x0012e52c) line 7060 + 13 bytes
js_Invoke(JSContext * 0x03c50818, unsigned int 0x00000001, unsigned int 0x00000001) line 1230 + 23 bytes
js_Interpret(JSContext * 0x03c50818, unsigned char * 0x03b47085, long * 0x0012ef2c) line 3292 + 15 bytes
js_Execute(JSContext * 0x03c50818, JSObject * 0x033614b8, JSScript * 0x03b47048, JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0012f034) line 1480 + 19 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x03c50818, JSObject * 0x033614b8, JSPrincipals * 0x03ebc2d4, const unsigned short * 0x09680048, unsigned int 0x009913fc, const char * 0x03667938, unsigned int 0x00000007, long * 0x0012f034) line 4107 + 25 bytes
nsJSContext::EvaluateString(const nsAString_internal & {...}, void * 0x033614b8, nsIPrincipal * 0x03ebc2d0, const char * 0x03667938, unsigned int 0x00000007, const char * 0x00500844 _js_default_str, nsAString_internal * 0x00000000, int * 0x0012f098) line 1074 + 67 bytes
nsScriptLoader::EvaluateScript(nsScriptLoadRequest * 0x03e8e340, const nsString & {...}) line 755
nsScriptLoader::ProcessRequest(nsScriptLoadRequest * 0x03e8e340) line 653 + 22 bytes
nsScriptLoader::DoProcessScriptElement(nsIScriptElement * 0x03dd91e4, nsIScriptLoaderObserver * 0x03dd91e0, int * 0x0012f6bc) line 586 + 23 bytes
nsScriptLoader::ProcessScriptElement(nsScriptLoader * const 0x03eb9018, nsIScriptElement * 0x03dd91e4, nsIScriptLoaderObserver * 0x03dd91e0) line 336 + 20 bytes
nsHTMLScriptElement::MaybeProcessScript() line 684 + 118 bytes
nsHTMLScriptElement::DoneAddingChildren(int 0x00000001) line 558
HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement * 0x03dd91c0, int 0x00000001) line 3862
SinkContext::CloseContainer(nsHTMLTag eHTMLTag_script) line 1382 + 23 bytes
HTMLContentSink::CloseContainer(HTMLContentSink * const 0x03d51614, nsHTMLTag eHTMLTag_script) line 2942 + 18 bytes
CNavDTD::CloseContainer(nsHTMLTag eHTMLTag_script) line 2736 + 31 bytes
CNavDTD::HandleEndToken(CToken * 0x03f2e968) line 1646 + 10 bytes
CNavDTD::HandleToken(CNavDTD * const 0x03e1be18, CToken * 0x03f2e968, nsIParser * 0x03eb27a8) line 742 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x03e1be18, nsIParser * 0x03eb27a8, nsITokenizer * 0x03df6950, nsITokenObserver * 0x00000000, nsIContentSink * 0x03d51614) line 375 + 20 bytes
nsParser::BuildModel(nsParser * const 0x03eb27a8) line 1921 + 34 bytes
nsParser::ResumeParse(int 0x00000001, int 0x00000001, int 0x00000001) line 1798 + 12 bytes
nsParser::OnStopRequest(nsParser * const 0x03eb27ac, nsIRequest * 0x03da65f0, nsISupports * 0x00000000, unsigned int 0x00000000) line 2479 + 21 bytes
nsDocumentOpenInfo::OnStopRequest(nsDocumentOpenInfo * const 0x03e07c90, nsIRequest * 0x03da65f0, nsISupports * 0x00000000, unsigned int 0x00000000) line 379
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x03d31f00, nsIRequest * 0x03da65f0, nsISupports * 0x00000000, unsigned int 0x00000000) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x03da65f8, nsIRequest * 0x03d99940, nsISupports * 0x00000000, unsigned int 0x00000000) line 4096
nsInputStreamPump::OnStateStop() line 512
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x03d99944, nsIAsyncInputStream * 0x03b70990) line 336 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x03e0365c) line 121
PL_HandleEvent(PLEvent * 0x03e0365c) line 688 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x010a9ec0) line 623 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x002203ac, unsigned int 0x0000c154, unsigned int 0x00000000, long 0x010a9ec0) line 1408 + 9 bytes
USER32! 77d48734()
USER32! 77d48816()
USER32! 77d489cd()
USER32! 77d48a10()
nsAppShell::Run(nsAppShell * const 0x03400730) line 135
nsAppStartup::Run(nsAppStartup * const 0x03400690) line 161 + 26 bytes
XRE_main(int 0x00000003, char * * 0x003f7220, const nsXREAppData * 0x0040301c kAppData) line 2321 + 35 bytes
main(int 0x00000003, char * * 0x003f7220) line 61 + 19 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

Comment 3

[Doug Wright]

(In reply to comment #2)
> I don't get the same stack as in the talkback report. In both 1.5 and trunk on
> winxp I see
> 
> 
>             /* Store kid in xml right away, to protect it from GC. */
> ->            XMLARRAY_SET_MEMBER(&xml->xml_kids, i, kid);
>             kid->parent = xml;
>             ++i;

That's also what I get in my debug xulrunner 1.8 build where I discovered the crash, but since I'm a novice when it comes to C/C++ I figured a talkback report would be more helpful. Apparently not...

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the obvious problems

This fixes the obvious problems that could cause crashes. I still saw crashing due to prematurely-collected things after this patch though, so something else is causing things to be unrooted or unreachable.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 209412 [details] [diff] [review]
Fix the obvious problems

This is good for now, thanks.  I think I'll evolve a way to return a value protected by the caller's local root scope, after you check this in.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix all problems

This fixes all problems with XMLARRAY_SET_MEMBER where it's being used in unsafe places.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 209460 [details] [diff] [review]
Fix all problems

General thought: make XMLARRAY_SET_MEMBER ensure that length is at least one greater than the index being assigned to.

>@@ -1725,32 +1721,30 @@ ParseNodeToXML(JSContext *cx, JSParseNod
>                                                     ATOM_KEY(pn2->pn_atom)));
>                     goto fail;
>                 }
>             }
> 
>             pn2 = pn2->pn_next;
>             JS_ASSERT(pn2);
>             JS_ASSERT(pn2->pn_type == TOK_XMLATTR);
> 
>             attr = js_NewXML(cx, JSXML_CLASS_ATTRIBUTE);
>-            if (!attr) {
>-                xml->xml_attrs.length = i;
>+            if (!attr)
>                 goto fail;
>-            }
> 
>             XMLARRAY_SET_MEMBER(&xml->xml_attrs, i, attr);
>+            xml->xml_attrs.length = i + 1;
>             attr->parent = xml;
>             attr->name = qn;
>             attr->xml_value = ATOM_TO_STRING(pn2->pn_atom);
>         }

Make the loop ending here a while ((pn2 = pn2->pn_next) != NULL) { loop with i = 0 set before, and use ++i above rather than i + 1 as the RHS to the assignment to xml_attrs.length.

>@@ -3234,25 +3228,25 @@ DeepCopySetInLRS(JSContext *cx, JSXMLArr
>             n > 1 && kid2->xml_class == JSXML_CLASS_TEXT) {
>             str = ChompXMLWhitespace(cx, kid2->xml_value);
>             if (!str) {
>                 to->length = j;
>                 return JS_FALSE;
>             }
>             kid2->xml_value = str;
>         }
> 
>         XMLARRAY_SET_MEMBER(to, j++, kid2);
>+        to->length = j;

Better to do ++j on the second line, I was free with side effects in macro params but it's not good style.

>@@ -3295,22 +3289,22 @@ DeepCopyInLRS(JSContext *cx, JSXML *xml,
>                 goto out;
>             for (i = 0; i < n; i++) {
>                 ns = XMLARRAY_MEMBER(&xml->xml_namespaces, i, JSXMLNamespace);
>                 ns2 = js_NewXMLNamespace(cx, ns->prefix, ns->uri, ns->declared);
>                 if (!ns2) {
>                     copy->xml_namespaces.length = i;

Don't need this if you set as you go, since it's initially 0 and if not, the previous iteration set it.

>                     ok = JS_FALSE;
>                     goto out;
>                 }
>                 XMLARRAY_SET_MEMBER(&copy->xml_namespaces, i, ns2);
>+                copy->xml_namespaces.length = i + 1;
>             }

Again I favor ++i and a while loop, for a general pattern.  Not as winning here as in the while ((pn2 = pn2->pn_next) != NULL) case earlier, but still...  Foolish consistency?

>@@ -6104,23 +6098,23 @@ xml_namespace(JSContext *cx, JSObject *o
>         if (!XMLArrayInit(cx, &inScopeNSes, length))
>             return JS_FALSE;
> 
>         for (i = 0; i < length; i++) {
>             ok = OBJ_GET_PROPERTY(cx, arrayobj, INT_TO_JSID(i), &v);
>             if (!ok)
>                 break;
>             JS_ASSERT(!JSVAL_IS_PRIMITIVE(v));
>             ns = (JSXMLNamespace *) JS_GetPrivate(cx, JSVAL_TO_OBJECT(v));
>             XMLARRAY_SET_MEMBER(&inScopeNSes, i, ns);
>+            inScopeNSes.length = i + 1;
>         }

Ditto.

Fix these, but consider the alterna-patch that ensures length bounds index in SET_MEMBER, and thereby removes all extra assignments to foo.length.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Alterna-patch

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 209496 [details] [diff] [review]
Alterna-patch

Great, thanks!

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using builds:

SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060127 Mozilla/1.0
-and-
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060128 Firefox/1.6a1

with the testcase URL at http://www.dougweb.org/bugzilla/324422.html (shouldn't we find a way to check that testcase into the tree?)

no longer crashing

Comment 12

[Bob Clary [:bc] (inactive)]

Checking in regress-324422-1.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-324422-1.js,v  <--  regress-324422-1.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/e4x/XML/regress-324422-2.js,v
done
Checking in regress-324422-2.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-324422-2.js,v  <--  regress-324422-2.js
initial revision: 1.1
done

note to self: regress-324422-2 crashes in js_HashString which is already filed.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 209496 [details] [diff] [review]
Alterna-patch

Do we want this on the 1.8.0 branch?

Comment 14

[Dave Liebreich [:davel]]

Comment on attachment 209496 [details] [diff] [review]
Alterna-patch

Yes, I think we want this on the 180 ranch.  Waiting for dveditz to double-check the patch and approve.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 209496 [details] [diff] [review]
Alterna-patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 17

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates) since in-testsuite+ indicates a test case exists in the js test library.

Comment 18

[Bob Clary [:bc] (inactive)]

I can't verify this. 

e4x/XML/regress-324422-1.js syntax error ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac
e4x/XML/regress-324422-2.js crash ff 1.8.0.1/1.8 linux

Blake, can you help me our here?

Comment 19

[Bob Clary [:bc] (inactive)]

This is looking good but I have some results I am not clear about. Waiting to verify with 20060303 builds.

Comment 20

[Jay Patel [:jay]]

Testcase in comment #1 doesn't crash for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060307 Firefox/1.5.0.2, so as soon as BC can verify this on his end, we should be good to go.

Comment 21

[Bob Clary [:bc] (inactive)]

v 1.8.0.2, 1.8, 1.9 20060307 win/linux/mac

Comment 22

[Bob Clary [:bc] (inactive)]

I am crashing in a trunk windows shell built on 20060328 on e4x/XML/regress-324422-2.js in opt and debug builds. I don't crash in the browser or on mac or linux.

My js stack doesn't have symbols for some reason so I can't tell where the crash occurs. 

Blake do you have a windows js build you can try to reproduce this with? Should I reopen this bug or file a new one?

Comment 23

[Bob Clary [:bc] (inactive)]

filed bug 332570 on the windows only shell crashes.

Comment 24

[Bob Clary [:bc] (inactive)]

note to self: outlier
test: e4x/XML/regress-324422-2.js: result: CRASHED   type: browser description: none : results/2006-07-06-13-51-48-firefox-trunk-dbg-1.9a1_2006070604-plum.mozilla.org.log CRASHED  6 (43.922962 seconds)

Comment 25

[Bob Clary [:bc] (inactive)]

note to self: e4x/XML/regress-324422-2.js, result: FAILED OUT OF MEMORY windows opt/dbg browser.