Closed
Bug 454704
Opened 16 years ago
Closed 16 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)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
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•16 years ago
|
Flags: blocking1.9.1+
Assignee | ||
Updated•16 years ago
|
Assignee: general → jim
Assignee | ||
Comment 1•16 years ago
|
||
I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.
Assignee | ||
Comment 2•16 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•16 years ago
|
Priority: -- → P2
Comment 3•16 years ago
|
||
Why is XPCSafeJSObjectWrapper is accessible to content?
Comment 4•16 years ago
|
||
Historical reasons.
Comment 5•16 years ago
|
||
(Namely, when it was originally implemented, it was available. I don't think there's a specific reason, though.)
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 6•16 years ago
|
||
Here's a simplified page that also crashes Firefox, without getting any windows involved.
Assignee | ||
Comment 7•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 16•16 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•16 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•16 years ago
|
||
Revised per Igor's suggestions.
Attachment #357022 -
Attachment is obsolete: true
Attachment #357244 -
Flags: review?(igor)
Attachment #357022 -
Flags: review?(igor)
Updated•16 years ago
|
Attachment #357244 -
Flags: review?(igor) → review+
Comment 19•16 years ago
|
||
I mark the bug as critical as it leads to double-free.
Whiteboard: [sg:critical?] → [sg:critical]
Comment 20•16 years ago
|
||
To Jim: can you land this to TraceMonkey?
Assignee | ||
Comment 21•16 years ago
|
||
(In reply to comment #20)
> To Jim: can you land this to TraceMonkey?
Certainly.
Assignee | ||
Comment 22•16 years ago
|
||
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 23•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 24•16 years ago
|
||
Updated•16 years ago
|
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Comment 25•16 years ago
|
||
Keywords: fixed1.9.1
Comment 26•16 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•16 years ago
|
Flags: blocking1.9.0.12?
Updated•16 years ago
|
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Updated•16 years ago
|
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey
Assignee | ||
Comment 27•16 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•16 years ago
|
Attachment #384503 -
Flags: review?(igor) → review+
Assignee | ||
Comment 28•16 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•16 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•16 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•16 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•16 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•15 years ago
|
Group: core-security
Comment 34•15 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•14 years ago
|
Crash Signature: [@ JS_HashTableRawLookup]
You need to log in
before you can comment on or make changes to this bug.
Description
•