Last Comment Bug 454113 - e4x/extensions/regress-374025.js - invalid write
: e4x/extensions/regress-374025.js - invalid write
: testcase, valgrind, verified1.8.1.18
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: x86 Linux
-- critical (vote)
: ---
Assigned To: Brian Crowder
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2008-09-07 16:15 PDT by Bob Clary [:bc:]
Modified: 2014-10-11 15:11 PDT (History)
9 users (show)
dveditz: blocking1.8.1.18+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

the backported patch (13.17 KB, patch)
2008-10-06 12:43 PDT, Brian Crowder
mrbkap: review-
Details | Diff | Splinter Review
v2 (13.86 KB, patch)
2008-10-07 12:07 PDT, Brian Crowder
mrbkap: review+
dveditz: approval1.8.1.18+
Details | Diff | Splinter Review
clean 1.8.0 patch (12.51 KB, patch)
2008-11-10 09:40 PST, Alexander Sack
no flags Details | Diff | Splinter Review

Description User image Bob Clary [:bc:] 2008-09-07 16:15:14 PDT
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)
Comment 1 User image Daniel Veditz [:dveditz] 2008-09-10 14:57:05 PDT
Is bug 374025 the trunk fix or is that unrelated?
Comment 2 User image Daniel Veditz [:dveditz] 2008-09-10 14:57:58 PDT
mm, no, that's closed WFM -- something else fixed it.
Comment 3 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-09-11 21:06:30 PDT
Adding sayrer for assignment help.
Comment 4 User image Brian Crowder 2008-10-01 15:56:38 PDT
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.
Comment 5 User image Brian Crowder 2008-10-06 11:45:28 PDT
Fixed on trunk by bug 410192 (thanks, hg bisect), more shortly.
Comment 6 User image Brian Crowder 2008-10-06 12:43:29 PDT
Created attachment 341958 [details] [diff] [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 7 User image Blake Kaplan (:mrbkap) 2008-10-06 20:26:36 PDT
Comment on attachment 341958 [details] [diff] [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) {
         sb->base = STRING_BUFFER_ERROR_BASE;
     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.
Comment 8 User image Brian Crowder 2008-10-07 12:07:15 PDT
Created attachment 342113 [details] [diff] [review]

Comment 9 User image Blake Kaplan (:mrbkap) 2008-10-07 12:21:43 PDT
Comment on attachment 342113 [details] [diff] [review]

Looks good. Also feel free to commit the .cvsignore change if it makes life easier for you when doing backports like this.
Comment 10 User image Daniel Veditz [:dveditz] 2008-10-09 09:54:01 PDT
Comment on attachment 342113 [details] [diff] [review]

Approved for, a=dveditz for release-drivers
Comment 11 User image Daniel Veditz [:dveditz] 2008-10-09 09:56:43 PDT
qawanted: We of course have the testcase that this is fixing, but I'm concerned that we test heavily the things this might break.
Comment 12 User image Brian Crowder 2008-10-20 08:44:52 PDT
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision:; previous revision:
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision:; previous revision:
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision:; previous revision:
Comment 13 User image Bob Clary [:bc:] 2008-10-25 10:20:10 PDT
Comment 14 User image Bob Clary [:bc:] 2008-10-29 23:44:22 PDT
crowder: this appears to have fixed e4x/extensions/regress-410192.js on 1.8.1 as well. Does that make sense to you?
Comment 15 User image Brian Crowder 2008-10-30 09:28:32 PDT
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.
Comment 16 User image Blake Kaplan (:mrbkap) 2008-10-30 10:49:52 PDT
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 17 User image Alexander Sack 2008-11-10 09:38:46 PST
Comment on attachment 342113 [details] [diff] [review]

a=asac for 1.8.0 (needs context adjustments to apply cleanly AND if possible without .cvsignore cruft :-P )
Comment 18 User image Alexander Sack 2008-11-10 09:40:11 PST
Created attachment 347308 [details] [diff] [review]
clean 1.8.0 patch
Comment 19 User image Joshua Mitchell (Inactive) 2014-10-11 15:11:21 PDT
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task

Note You need to log in before you can comment on or make changes to this bug.