Bugzilla@Mozilla – Bug 454113
e4x/extensions/regress-374025.js - invalid write
Last modified: 2008-11-12 17:32:21 PST
Summon comment box
This is fixed on the trunk and is a known crasher on 1.8.1, but I just realized it is a security issue. ==30135== Invalid write of size 2 ==30135== at 0x40D6CA4: js_RepeatChar (jsscan.c:845) ==30135== by 0x40F5597: XMLToXMLString (jsxml.c:2709) ==30135== by 0x40F6246: XMLToXMLString (jsxml.c:3004) ==30135== by 0x40F6624: ToXMLString (jsxml.c:3082) ==30135== by 0x4102289: xml_toXMLString (jsxml.c:7168) ==30135== by 0x406B298: js_Invoke (jsinterp.c:1387) ==30135== by 0x406B6CB: js_InternalInvoke (jsinterp.c:1481) ==30135== by 0x40A78ED: js_TryMethod (jsobj.c:4686) ==30135== by 0x40E8B98: js_ValueToSource (jsstr.c:2718) ==30135== by 0x40E1CA2: str_uneval (jsstr.c:470) ==30135== by 0x406B298: js_Invoke (jsinterp.c:1387) ==30135== by 0x407E78D: js_Interpret (jsinterp.c:3964)
Is bug 374025 the trunk fix or is that unrelated?
mm, no, that's closed WFM -- something else fixed it.
Adding sayrer for assignment help.
Igor: Any idea what trunk work might've fixed this? I'll be happy to port. If not, don't spend any more time on it, I'll track it down.
Fixed on trunk by bug 410192 (thanks, hg bisect), more shortly.
Created attachment 341958 [details] [review] the backported patch Took the interesting code from the fix in bug 410192 and ported to 1.8, asking mrbkap for review since he wrote the original code.
Comment on attachment 341958 [details] [review] the backported patch - In AppendAttributeValue(JSContext *cx, JSStringBuffer *sb, JSString *valstr) { - js_AppendCString(sb, "=\""); - valstr = js_EscapeAttributeValue(cx, valstr); + js_AppendChar(sb, '='); + valstr = js_EscapeAttributeValue(cx, valstr, JS_TRUE); if (!valstr) { free(sb->base); sb->base = STRING_BUFFER_ERROR_BASE; return; } js_AppendJSString(sb, valstr); js_AppendChar(sb, '"'); The last js_AppendChar dies in brendan's patch but lives on here. I see you removing the first " (for the one in js_EscapeAttributeValue) so it seems like this one should go too. r- until that's addressed.
Created attachment 342113 [details] [review] v2 Thanks!
Comment on attachment 342113 [details] [review] v2 Looks good. Also feel free to commit the .cvsignore change if it makes life easier for you when doing backports like this.
Comment on attachment 342113 [details] [review] v2 Approved for 1.8.1.18, a=dveditz for release-drivers
qawanted: We of course have the testcase that this is fixing, but I'm concerned that we test heavily the things this might break.
Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.181.2.104; previous revision: 3.181.2.103 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.72; previous revision: 3.50.2.71 done Checking in jsxml.h; /cvsroot/mozilla/js/src/jsxml.h,v <-- jsxml.h new revision: 3.10.4.5; previous revision: 3.10.4.4 done
v 1.8.1.18
crowder: this appears to have fixed e4x/extensions/regress-410192.js on 1.8.1 as well. Does that make sense to you?
It makes sense that my change -could- have fixed that, but I didn't expect it and I don't know for sure exactly why. Let's call it serendipity, as I don't really have time to investigate it.
I looked into it -- this is because toSource on an XML attribute is expected to produce quotes around the value and toString isn't. This change enforces that (if you follow the TO_SOURCE_FLAG propagation through XMLToXMLString). So in summary: it makes sense for this to have fixed that test.
Comment on attachment 342113 [details] [review] v2 a=asac for 1.8.0 (needs context adjustments to apply cleanly AND if possible without .cvsignore cruft :-P )
Created attachment 347308 [details] [review] clean 1.8.0 patch