Closed
Bug 372309
Opened 17 years ago
Closed 17 years ago
Crash in [@SetArrayElement] using canvas
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: sharparrow1, Assigned: crowderbt)
References
Details
(Keywords: fixed1.8.0.15, testcase, verified1.8.1.8, Whiteboard: [sg:critical?])
Attachments
(3 files, 6 obsolete files)
521 bytes,
text/html
|
Details | |
1.07 KB,
patch
|
crowderbt
:
review+
dveditz
:
approval1.8.1.8+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
2.72 KB,
text/plain
|
Details |
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.)
Reporter | ||
Comment 1•17 years ago
|
||
Hmm, should have simplified before posting originally... pretty simple :(
Attachment #256958 -
Attachment is obsolete: true
Reporter | ||
Updated•17 years ago
|
Attachment #256961 -
Attachment description: Simplified testcase → Simplified testcase (WARNING: CRASHES INSTANTLY)
Assignee | ||
Comment 2•17 years ago
|
||
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.
Assignee: nobody → crowder
Severity: normal → critical
Component: Layout: Canvas → JavaScript Engine
Whiteboard: [sg:critical?]
Comment 3•17 years ago
|
||
Trunk-only, though. /be
Updated•17 years ago
|
Assignee | ||
Comment 4•17 years ago
|
||
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.
Attachment #256968 -
Flags: review?(brendan)
Comment 5•17 years ago
|
||
(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.
Assignee | ||
Comment 6•17 years ago
|
||
Thanks for the feedback, Igor.
Attachment #256968 -
Attachment is obsolete: true
Attachment #256975 -
Flags: review?(igor)
Attachment #256968 -
Flags: review?(brendan)
Comment 7•17 years ago
|
||
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;
Attachment #256975 -
Flags: review?(igor) → review+
Comment 8•17 years ago
|
||
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.
Attachment #256975 -
Flags: review+ → review-
Assignee | ||
Comment 9•17 years ago
|
||
This is the patch I will land this evening, barring further feedback. Thanks, Igor.
Attachment #256975 -
Attachment is obsolete: true
Attachment #256976 -
Flags: review+
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #256976 -
Attachment is obsolete: true
Attachment #256977 -
Flags: review+
Assignee | ||
Comment 11•17 years ago
|
||
Sorry for the bugspam
Attachment #256977 -
Attachment is obsolete: true
Attachment #256978 -
Flags: review+
Comment 12•17 years ago
|
||
(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.
Updated•17 years ago
|
Attachment #256978 -
Flags: review+
Assignee | ||
Comment 13•17 years ago
|
||
The last patch doesn't have the additional issue of leaving tvr pushed, either.
Status: NEW → ASSIGNED
Comment 14•17 years ago
|
||
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
Assignee | ||
Comment 15•17 years ago
|
||
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
Attachment #256978 -
Attachment is obsolete: true
Attachment #256989 -
Flags: review+
Updated•17 years ago
|
Flags: blocking1.9?
Whiteboard: [sg:critical?] → [sg:critical?] 1.9-only
Assignee | ||
Comment 16•17 years ago
|
||
jsarray.c: 3.107
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite+
QA Contact: layout.canvas → general
Comment 17•17 years ago
|
||
Updated•17 years ago
|
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Updated•17 years ago
|
Whiteboard: [sg:critical?] 1.9-only → [sg:critical?] post 1.8-branch
Updated•17 years ago
|
Group: security
Comment 19•17 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-372309.js,v <-- regress-372309.js initial revision: 1.1
Comment 20•17 years ago
|
||
The bug exists on 1.8.1 branch, for details see bug 360701.
Comment 21•17 years ago
|
||
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.
Attachment #256989 -
Flags: approval1.8.1.8?
Updated•17 years ago
|
Flags: blocking1.8.0.14? → blocking1.8.1.8?
Updated•17 years ago
|
Flags: blocking1.8.1.8? → blocking1.8.1.8+
Comment 22•17 years ago
|
||
Comment on attachment 256989 [details] [diff] [review] trust the macro! approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #256989 -
Flags: approval1.8.1.8? → approval1.8.1.8+
Comment 23•17 years ago
|
||
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
Keywords: fixed1.8.1.8
Comment 24•17 years ago
|
||
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
Keywords: fixed1.8.1.8 → verified1.8.1.8
Comment 25•16 years ago
|
||
Comment on attachment 256989 [details] [diff] [review] trust the macro! a=asac for 1.8.0.15
Attachment #256989 -
Flags: approval1.8.0.15+
Comment 26•16 years ago
|
||
please land on 1.8.0 branch.
Flags: blocking1.8.0.15+
Keywords: checkin-needed
Comment 27•16 years ago
|
||
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
Keywords: checkin-needed → fixed1.8.0.15
You need to log in
before you can comment on or make changes to this bug.
Description
•