Closed
Bug 454704
Opened 15 years ago
Closed 15 years ago
Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jruderman, Assigned: jimb)
Details
(4 keywords, Whiteboard: [sg:critical] fixed-in-tracemonkey)
Crash Data
Attachments
(5 files, 1 obsolete file)
239 bytes,
text/html
|
Details | |
262 bytes,
text/html
|
Details | |
1.93 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
2.86 KB,
text/plain
|
Details | |
1.96 KB,
patch
|
igor
:
review+
samuel.sidler+old
:
approval1.9.0.12+
|
Details | Diff | Splinter Review |
Thread 0 Crashed: 0 JS_HashTableRawLookup + 10 (jshash.cpp:177) 1 MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 120 (jsobj.cpp:360) 2 MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 750 (jsobj.cpp:418) 3 MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 750 (jsobj.cpp:418) 4 js_EnterSharpObject + 277 (jsobj.cpp:475) 5 js_array_join_sub + 141 (jsarray.cpp:1214) 6 array_toString(JSContext*, unsigned int, long*) + 165 (jsarray.cpp:1429) 7 js_Invoke + 1313 (jsinterp.cpp:1189) ...
Updated•15 years ago
|
Flags: blocking1.9.1+
Assignee | ||
Updated•15 years ago
|
Assignee: general → jim
Assignee | ||
Comment 1•15 years ago
|
||
I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.
Assignee | ||
Comment 2•15 years ago
|
||
What's interesting is that we seem to have invoked the getter for f, instead of just registering it in the table: (gdb) where 30 #0 MarkSharpObjects (cx=0xb2a69800, obj=0xb14295a0, idap=0xbfff7a24) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:386 #1 0xb7e44432 in js_EnterSharpObject (cx=0xb2a69800, obj=0xb14295a0, idap=0x0, sp=0xbfff7ae4) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:483 #2 0xb7db0f03 in array_join_sub (cx=0xb2a69800, obj=0xb14295a0, op=TO_SOURCE, sep=0x0, rval=0xb15aa0f4) at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1251 #3 0xb7db1c0e in array_toSource (cx=0xb2a69800, argc=0, vp=0xb15aa0f4) at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1452 #4 0xb7e2a52b in js_Invoke (cx=0xb2a69800, argc=0, vp=0xb15aa0f4, flags=0) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1192 #5 0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb14295a0, fval=-1321007752, flags=0, argc=0, argv=0x0, rval=0xbfff934c) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388 #6 0xb7e2b3ea in js_InternalGetOrSet (cx=0xb2a69800, obj=0xb14295a0, id=-1238794948, fval=-1321007752, mode=JSACC_READ, argc=0, argv=0x0, rval=0xbfff934c) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1449 #7 0xb7e3b3a2 in js_NativeGet (cx=0xb2a69800, obj=0xb14295a0, pobj=0xb14295a0, sprop=0xb1484130, vp=0xbfff934c) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3669 #8 0xb7e3e174 in js_GetPropertyHelper (cx=0xb2a69800, obj=0xb14295a0, id=-1238794948, vp=0xbfff934c, entryp=0x0) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3818 #9 0xb7e3e226 in js_GetProperty (cx=0xb2a69800, obj=0xb14295a0, id=-1238794948, vp=0xbfff934c) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3830 #10 0xb7dff62c in js_Interpret (cx=0xb2a69800) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:4688 #11 0xb7e2a984 in js_Invoke (cx=0xb2a69800, argc=1, vp=0xb15aa0d4, flags=0) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1331 #12 0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb14295a0, fval=-1321005232, flags=0, argc=1, argv=0xbfff9780, rval=0xbfff978c) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388 #13 0xb7d936cf in JS_CallFunctionValue (cx=0xb2a69800, obj=0xb14295a0, fval=-1321005232, argc=1, argv=0xbfff9780, rval=0xbfff978c) at /home/jimb/mc/b/454704/src/js/src/jsapi.cpp:5242 #14 0xb613d4a8 in XPC_SJOW_GetOrSetProperty (cx=0xb2a69800, obj=0xb1429600, id=-1238794948, vp=0xbfff98f8, aIsSet=0) at /home/jimb/mc/b/454704/src/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp:585 #15 0xb613d558 in XPC_SJOW_GetProperty (cx=0xb2a69800, obj=0xb1429600, id=-1238794948, vp=0xbfff98f8) at /home/jimb/mc/b/454704/src/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp:593 #16 0xb7e3b432 in js_NativeGet (cx=0xb2a69800, obj=0xb1429600, pobj=0xb1429600, sprop=0xb1484290, vp=0xbfff98f8) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3669 #17 0xb7e3e174 in js_GetPropertyHelper (cx=0xb2a69800, obj=0xb1429600, id=-1238794948, vp=0xbfff98f8, entryp=0x0) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3818 #18 0xb7e3e226 in js_GetProperty (cx=0xb2a69800, obj=0xb1429600, id=-1238794948, vp=0xbfff98f8) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3830 #19 0xb7e441d7 in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429600, idap=0x0) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:417 #20 0xb7e4423e in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429580, idap=0x0) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:426 #21 0xb7e4423e in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429560, idap=0xbfff9a74) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:426 #22 0xb7e44432 in js_EnterSharpObject (cx=0xb2a69800, obj=0xb1429560, idap=0x0, sp=0xbfff9b34) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:483 #23 0xb7db0f03 in array_join_sub (cx=0xb2a69800, obj=0xb1429560, op=TO_STRING, sep=0x0, rval=0xb15aa0cc) at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1251 #24 0xb7db1b58 in array_toString (cx=0xb2a69800, argc=0, vp=0xb15aa0cc) at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1466 #25 0xb7e2a52b in js_Invoke (cx=0xb2a69800, argc=0, vp=0xb15aa0cc, flags=0) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1192 #26 0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb1429560, fval=-1321007696, flags=0, argc=0, argv=0x0, rval=0xbfff9d54) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388 #27 0xb7e3934e in js_TryMethod (cx=0xb2a69800, obj=0xb1429560, atom=0xb6af1264, argc=0, argv=0x0, rval=0xbfff9d54) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:4963 #28 0xb7e3a441 in js_DefaultValue (cx=0xb2a69800, obj=0xb1429560, hint=JSTYPE_VOID, vp=0xb15aa0b4) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:4197 #29 0xb7df5f0c in js_Interpret (cx=0xb2a69800) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:3778 (More stack frames follow...) (gdb)
Updated•15 years ago
|
Priority: -- → P2
Comment 3•15 years ago
|
||
Why is XPCSafeJSObjectWrapper is accessible to content?
Comment 4•15 years ago
|
||
Historical reasons.
Comment 5•15 years ago
|
||
(Namely, when it was originally implemented, it was available. I don't think there's a specific reason, though.)
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 6•15 years ago
|
||
Here's a simplified page that also crashes Firefox, without getting any windows involved.
Assignee | ||
Comment 7•15 years ago
|
||
This seems to do the trick. Alternatively, should OBJ_GET_ATTRIBUTES carry getter and setter flags through wrappers, so we can avoid invoking them altogether? (Are there ops that a wrapper can use to fetch getters and setters, instead of invoking them?)
Attachment #357022 -
Flags: review?(sayrer)
Assignee | ||
Comment 8•15 years ago
|
||
With that patch applied, Firefox no longer crashes, but we do get bug 473630. So there's more to be sorted out here.
Comment 9•15 years ago
|
||
Comment on attachment 357022 [details] [diff] [review] Bug 454704: Protect sharpObjectMap in the presence of wrapped getters. >Bug 454704: Protect sharpObjectMap in the presence of wrapped getters. > >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >--- a/js/src/jsobj.cpp >+++ b/js/src/jsobj.cpp >@@ -381,12 +381,20 @@ MarkSharpObjects(JSContext *cx, JSObject > * itself badly. Without this fix, if we reenter the basis case where > * map->depth == 0, when unwinding the inner call we will destroy the > * newly-created hash table and crash. >+ * >+ * Note that wrapped objects may have getters, which we'll >+ * invoke instead of simply marking, since we won't recognize >+ * the object as a native object. Those getters could start a >+ * sharp-marking traversal themselves. So, leave depth >+ * incremented across all property gets, as well. > */ > ++map->depth; > ida = JS_Enumerate(cx, obj); >- --map->depth; > if (!ida) >- return NULL; >+ { >+ --map->depth; >+ return NULL; >+ } Please turn off the Stallman-free-software-song elisp (or whatever) that is foisting this over-indented and alien style on braced consequents :-/. Igor, could you review? IIRC you have relatively recent experience (GC-oriented) here. Or maybe mrbkap could r+ if you are swamped. Thanks, /be
Attachment #357022 -
Flags: review?(sayrer) → review?(igor)
Comment 10•15 years ago
|
||
Actually, mrbkap has the blame, but it was a long time ago: revision 3.207 date: 2005/08/12 00:46:56; author: mrbkap%gmail.com; state: Exp; lines: +8 -0 bug 304376: Fix hash table refcounting problem while recursively entering a marked object. r+a=brendan and I'm r+a bottle-inspector (http://www.thesimpsons.com/episode_guide/0416.htm, "good, good, rat, good, ..." [Hitler's head in a jar goes by as inspector gabs with Homer]). Sorry for doing a Duff-ish job there. /be
Comment 11•15 years ago
|
||
(In reply to comment #9) > (From update of attachment 357022 [details] [diff] [review]) > >Bug 454704: Protect sharpObjectMap in the presence of wrapped getters. It is better to move map->depth++/-- and related comments from MarkSharpObjects and put them around the line where js_EnterSharpObject calls MarkSharpObjects. This not only make clear what the comments are talking about but also avoid duplicated map->depth-- in MarkSharpObjects due to an early return. For an extra safety MarkSharpObjects should also assert that map->depth >= 1.
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #9) > Please turn off the Stallman-free-software-song elisp (or whatever) that is > foisting this over-indented and alien style on braced consequents :-/. Fixed. Emacs doesn't decide which line the braces go on; my fingers will get reconfigured soon enough.
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #11) > It is better to move map->depth++/-- and related comments from MarkSharpObjects > and put them around the line where js_EnterSharpObject calls MarkSharpObjects. > This not only make clear what the comments are talking about but also avoid > duplicated map->depth-- in MarkSharpObjects due to an early return. For an > extra safety MarkSharpObjects should also assert that map->depth >= 1. So, this is interesting: in the case of a nested traversal like the one described in the comment added by the patch, because depth would already be > 0, js_EnterSharpObject won't call MarkSharpObjects at all. It assumes the first MarkSharpObjects' traversal visited everything there was to visit, which isn't true.
Comment 14•15 years ago
|
||
(In reply to comment #12) > (In reply to comment #9) > > Please turn off the Stallman-free-software-song elisp (or whatever) that is > > foisting this over-indented and alien style on braced consequents :-/. > > Fixed. Emacs doesn't decide which line the braces go on; my fingers will get > reconfigured soon enough. Thanks -- jwz always blamed his elisp for (two-space c-basic-offset) RMSstyle in the Netscape XFE. /be
Comment 15•15 years ago
|
||
(In reply to comment #13) > in the case of a nested traversal... because depth would already be > > 0, js_EnterSharpObject won't call MarkSharpObjects at all. It assumes the > first MarkSharpObjects' traversal visited everything there was to visit, which > isn't true. This leads to printing of null for values value of some properties of non-native (is SM sense) objects. This should be fine for the fix as without the patch the code just crashes as a result of double-free.
Group: core-security
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #15) > This leads to printing of null for values value of some properties of > non-native (is SM sense) objects. This should be fine for the fix as without > the patch the code just crashes as a result of double-free. It doesn't lead to a failure to detect cycles in the nested traversal? (I'm not sure I can parse the first sentence.)
Comment 17•15 years ago
|
||
(In reply to comment #16) > It doesn't lead to a failure to detect cycles in the nested traversal? You are right, it will lead to infinite recursion when !OBJ_IS_NATIVE() happens twice in the object graph, not printing NULL or something. But that is fine as a workaround. At some point the recursion detection will be triggered and an error is reported. This is very similar to what one get when using custom toSource implementation like in: js> var point = { toSource: function() { return "(" + uneval(this.x) + ", " + uneval(this.y)+")"; } }; js> point.x = 10 10 js> point.y = point; [object Object] js> uneval(point) typein:8: InternalError: too much recursion Such error message is infinitely better than the crash via double-free that happens with the current code.
Assignee | ||
Comment 18•15 years ago
|
||
Revised per Igor's suggestions.
Attachment #357022 -
Attachment is obsolete: true
Attachment #357244 -
Flags: review?(igor)
Attachment #357022 -
Flags: review?(igor)
Updated•15 years ago
|
Attachment #357244 -
Flags: review?(igor) → review+
Comment 19•15 years ago
|
||
I mark the bug as critical as it leads to double-free.
Whiteboard: [sg:critical?] → [sg:critical]
Comment 20•15 years ago
|
||
To Jim: can you land this to TraceMonkey?
Assignee | ||
Comment 21•15 years ago
|
||
(In reply to comment #20) > To Jim: can you land this to TraceMonkey? Certainly.
Assignee | ||
Comment 22•15 years ago
|
||
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 23•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/73f9f5e01fef
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 24•15 years ago
|
||
Updated•15 years ago
|
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Comment 25•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/da90ad6653c2
Keywords: fixed1.9.1
Comment 26•15 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•15 years ago
|
Flags: blocking1.9.0.12?
Updated•15 years ago
|
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Updated•15 years ago
|
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey
Assignee | ||
Comment 27•15 years ago
|
||
Adapt fix for 1.9.0 tree. Verified that Firefox crashes without patch, doesn't crash with.
Attachment #384503 -
Flags: review?(igor)
Attachment #384503 -
Flags: approval1.9.0.12?
Updated•15 years ago
|
Attachment #384503 -
Flags: review?(igor) → review+
Assignee | ||
Comment 28•15 years ago
|
||
Thanks, Igor! A favor: I haven't got my SSH machinery updated for committing to CVS; could you land that patch for me?
Comment 29•15 years ago
|
||
(In reply to comment #28) > Thanks, Igor! A favor: I haven't got my SSH machinery updated for committing > to CVS; could you land that patch for me? It is to late for me to do it today. But I will land tomorrow early morning PDT if this gets an approval.
Comment 30•15 years ago
|
||
Comment on attachment 384503 [details] [diff] [review] Bug 464704: Adapt fix for 1.9.0 tree. Approved for 1.9.0.12. a=ss
Attachment #384503 -
Flags: approval1.9.0.12? → approval1.9.0.12+
Comment 31•15 years ago
|
||
landed to 1.9.0 : Checking in jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.485; previous revision: 3.484 done
Keywords: fixed1.9.0.12
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
Comment 32•15 years ago
|
||
Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). Testcases both crash on 1.9.0.11 but not with 1.9.0.12 build.
Keywords: fixed1.9.0.12 → verified1.9.0.12
Updated•14 years ago
|
Group: core-security
Comment 34•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/e0017d17ac44 /cvsroot/mozilla/js/tests/js1_5/extensions/regress-454704.js,v <-- regress-454704.js initial revision: 1.1
Updated•13 years ago
|
Crash Signature: [@ JS_HashTableRawLookup]
You need to log in
before you can comment on or make changes to this bug.
Description
•