Last Comment Bug 372309 - Crash in [@SetArrayElement] using canvas
: Crash in [@SetArrayElement] using canvas
Status: VERIFIED FIXED
[sg:critical?]
: fixed1.8.0.15, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks: 310405 360701
  Show dependency treegraph
 
Reported: 2007-03-01 13:49 PST by Eli Friedman
Modified: 2008-03-08 05:42 PST (History)
9 users (show)
dveditz: blocking1.9?
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x-
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x-
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (not simplified) (4.07 KB, text/html)
2007-03-01 13:49 PST, Eli Friedman
no flags Details
Simplified testcase (WARNING: CRASHES INSTANTLY) (521 bytes, text/html)
2007-03-01 14:14 PST, Eli Friedman
no flags Details
temp root for array object (943 bytes, patch)
2007-03-01 16:04 PST, Brian Crowder
no flags Details | Diff | Splinter Review
better behavior (1.02 KB, patch)
2007-03-01 16:34 PST, Brian Crowder
igor: review-
Details | Diff | Splinter Review
igor's nit (1.09 KB, patch)
2007-03-01 17:07 PST, Brian Crowder
crowderbt: review+
Details | Diff | Splinter Review
tweak to kill a compiler warning (1.09 KB, patch)
2007-03-01 17:09 PST, Brian Crowder
crowderbt: review+
Details | Diff | Splinter Review
with the right patch this time (1.11 KB, patch)
2007-03-01 17:10 PST, Brian Crowder
crowderbt: review+
igor: review+
Details | Diff | Splinter Review
trust the macro! (1.07 KB, patch)
2007-03-01 20:21 PST, Brian Crowder
crowderbt: review+
dveditz: approval1.8.1.8+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
js1_5/extensions/regress-372309.js (2.72 KB, text/plain)
2007-03-06 00:15 PST, Bob Clary [:bc:]
no flags Details

Description Eli Friedman 2007-03-01 13:49:53 PST
Created attachment 256958 [details]
Testcase (not simplified)

WARNING: Frame IP not in any known module. Following frames may be wrong.
0xcdcdcd20
js3250!SetArrayElement(struct JSContext * cx = 0x02d80d58, struct JSObject * obj = 0x03afa5c0, unsigned long index = 0x12e5dc, long v = 511)+0x5e [c:\mozilla\mozilla\js\src\jsarray.c @ 299]
js3250!InitArrayElements(struct JSContext * cx = 0x000c57d1, struct JSObject * obj = 0x03afa5c0, unsigned long start = 0x62be8, unsigned long end = 0x9c400, long * vector = 0x03afae9c)+0x39 [c:\mozilla\mozilla\js\src\jsarray.c @ 738]
js3250!InitArrayObject(struct JSContext * cx = 0x03afae9c, struct JSObject * obj = 0x03afa5c0, unsigned long length = 0x9c400, long * vector = 0x08490040)+0x61 [c:\mozilla\mozilla\js\src\jsarray.c @ 762]
js3250!js_NewArrayObject(struct JSContext * cx = 0x002c5dd7, unsigned long length = 0xff000000, long * vector = 0x0012e7e4)+0x31 [c:\mozilla\mozilla\js\src\jsarray.c @ 1997]
gklayout!nsCanvasRenderingContext2D::GetImageData(void)+0x2fc [c:\mozilla\mozilla\content\canvas\src\nscanvasrenderingcontext2d.cpp @ 3077]
xpcom_core!NS_InvokeByIndex(class nsISupports * that = 0x063ff9c8, unsigned int methodIndex = 0x37, unsigned int paramCount = 0, struct nsXPTCVariant * params = 0x0012e6b8)+0x27 [c:\mozilla\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102]
xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x00b78aa8, XPCWrappedNative::CallMode mode = 12028584 (No matching enumerant))+0x7f2 [c:\mozilla\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2251]

Seems like an evil enough crash that it might be worth filing as a security bug.

To reproduce, load up two images into the testcase (don't know of any images off the top of my head on b.m.o to refer to; you can save the testcase and put the urls of two local files in.)  Then, click the compare button.  Then, click it again.  Watch it explode.  (Testcase could probably be reduced more, but I haven't done it yet.)
Comment 1 Eli Friedman 2007-03-01 14:14:04 PST
Created attachment 256961 [details]
Simplified testcase (WARNING: CRASHES INSTANTLY)

Hmm, should have simplified before posting originally... pretty simple :(
Comment 2 Brian Crowder 2007-03-01 15:04:54 PST
bz thinks, and I agree, that this is a problem of the new array object not having a root and InitArrayElements CHECK_OPERATION_LIMIT potentially yielding a GC (or many other) operations which smash the array's newborn root (if there ever is one in this case??).  I'll take a stab at a patch...

Since it is a GC-hazard, I feel that it most certainly should be considered a critical security bug.
Comment 3 Brendan Eich [:brendan] 2007-03-01 15:15:36 PST
Trunk-only, though.

/be
Comment 4 Brian Crowder 2007-03-01 16:04:18 PST
Created attachment 256968 [details] [diff] [review]
temp root for array object

Not sure if this is correct, but seems to be the common technique.  Whoever the array object is passed to will be responsible for rooting it again.
Comment 5 Igor Bukanov 2007-03-01 16:15:10 PST
(In reply to comment #4)
> Created an attachment (id=256968) [details]
> temp root for array object
> 
> Not sure if this is correct, but seems to be the common technique.  Whoever the
> array object is passed to will be responsible for rooting it again.
> 

For complete safety it is necessary to put obj into newborn array to restore invariant that the callback can break.
Comment 6 Brian Crowder 2007-03-01 16:34:29 PST
Created attachment 256975 [details] [diff] [review]
better behavior

Thanks for the feedback, Igor.
Comment 7 Igor Bukanov 2007-03-01 16:59:38 PST
Comment on attachment 256975 [details] [diff] [review]
better behavior

> JSObject *
> js_NewArrayObject(JSContext *cx, jsuint length, jsval *vector)
> {
...
>+    tvr.u.value = OBJECT_TO_JSVAL(obj);
>     if (!InitArrayObject(cx, obj, length, vector)) {
>         cx->weakRoots.newborn[GCX_OBJECT] = NULL;
>         return NULL;
>     }
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+
>+    /* Set newborn root, in case we lost it.  */
>+    cx->weakRoots.newborn[GCX_OBJECT] = obj;
>     return obj;

Nit: the if (!InitArrayObject) part can be written as:

if (!InitArrayObject(cx, obj, length, vector))
  obj = NULL;
Comment 8 Igor Bukanov 2007-03-01 17:04:15 PST
Comment on attachment 256975 [details] [diff] [review]
better behavior

>+    JS_PUSH_TEMP_ROOT_OBJECT(cx, obj, &tvr);
>+    tvr.u.value = OBJECT_TO_JSVAL(obj);
>     if (!InitArrayObject(cx, obj, length, vector)) {
>         cx->weakRoots.newborn[GCX_OBJECT] = NULL;
>         return NULL;

Non-nit: this will leave tvr pushed.
Comment 9 Brian Crowder 2007-03-01 17:07:24 PST
Created attachment 256976 [details] [diff] [review]
igor's nit

This is the patch I will land this evening, barring further feedback.  Thanks, Igor.
Comment 10 Brian Crowder 2007-03-01 17:09:36 PST
Created attachment 256977 [details] [diff] [review]
tweak to kill a compiler warning
Comment 11 Brian Crowder 2007-03-01 17:10:53 PST
Created attachment 256978 [details] [diff] [review]
with the right patch this time

Sorry for the bugspam
Comment 12 Igor Bukanov 2007-03-01 17:12:09 PST
(In reply to comment #10)
> Created an attachment (id=256977) [details]
> tweak to kill a compiler warning
> 

The diff shows that this patch and the previous are the same.
Comment 13 Brian Crowder 2007-03-01 17:13:41 PST
The last patch doesn't have the additional issue of leaving tvr pushed, either.
Comment 14 Brendan Eich [:brendan] 2007-03-01 18:08:05 PST
Comment on attachment 256978 [details] [diff] [review]
with the right patch this time

>+    JS_PUSH_TEMP_ROOT_OBJECT(cx, obj, &tvr);
>+    tvr.u.value = OBJECT_TO_JSVAL(obj);

The macro sets u.value for you.

>+    if (!InitArrayObject(cx, obj, length, vector))
>+        obj = NULL;
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+
>+    /* Set/clear newborn root, in case we lost it.  */
>+    cx->weakRoots.newborn[GCX_OBJECT] = (JSGCThing *) obj;

Need to get rid of newborn roots, but that's another bug (on Igor's list IIRC).

/be
Comment 15 Brian Crowder 2007-03-01 20:21:47 PST
Created attachment 256989 [details] [diff] [review]
trust the macro!

ugh, I went and found that macro so I could get rid of that line then didn't get rid of the line.  Juggling too many patches today.  Thanks, Brendan
Comment 16 Brian Crowder 2007-03-02 09:36:28 PST
jsarray.c: 3.107
Comment 17 Bob Clary [:bc:] 2007-03-06 00:15:01 PST
Created attachment 257483 [details]
js1_5/extensions/regress-372309.js
Comment 18 Bob Clary [:bc:] 2007-03-21 15:31:30 PDT
verified fixed 1.9.0 20070320 win/mac*/linux
Comment 19 Bob Clary [:bc:] 2007-09-21 07:12:30 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-372309.js,v  <--  regress-372309.js
initial revision: 1.1
Comment 20 Igor Bukanov 2007-09-28 14:43:10 PDT
The bug exists on 1.8.1 branch, for details see bug 360701.
Comment 21 Igor Bukanov 2007-09-28 14:44:58 PDT
Comment on attachment 256989 [details] [diff] [review]
trust the macro!

The patch must go to 1.8.1 branch to fix the GC hazard there.
Comment 22 Daniel Veditz [:dveditz] 2007-09-28 16:25:33 PDT
Comment on attachment 256989 [details] [diff] [review]
trust the macro!

approved for 1.8.1.8, a=dveditz for release-drivers
Comment 23 Igor Bukanov 2007-09-28 16:32:15 PDT
I checked in the patch from comment 15 to MOZILLA_1_8_BRANCH:

Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.26; previous revision: 3.58.2.25
done
Comment 24 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-09-29 16:39:42 PDT
verified fixed 1.8.1.8 using the testcase from comment #1 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8pre) Gecko/2007092903 BonEcho/2.0.0.8pre on Fedora F7 and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.8pre) Gecko/20070927 BonEcho/2.0.0.8pre ID:2007092704 - no crash on testcase

-> Adding verified keyword
Comment 25 Alexander Sack 2008-02-28 06:47:34 PST
Comment on attachment 256989 [details] [diff] [review]
trust the macro!

a=asac for 1.8.0.15
Comment 26 Alexander Sack 2008-02-28 07:52:45 PST
please land on 1.8.0 branch.
Comment 27 Reed Loden [:reed] (use needinfo?) 2008-03-08 05:42:44 PST
MOZILLA_1_8_0_BRANCH:

Checking in js/src/jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.10.2.13; previous revision: 3.58.2.10.2.12
done

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