Closed Bug 454704 Opened 12 years ago Closed 12 years ago

Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource

Categories

(Core :: JavaScript Engine, defect, P2)

x86
macOS
defect

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)

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)
...
Flags: blocking1.9.1+
Assignee: general → jim
I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.
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)
Priority: -- → P2
Why is XPCSafeJSObjectWrapper is accessible to content?
Historical reasons.
(Namely, when it was originally implemented, it was available. I don't think there's a specific reason, though.)
Flags: in-testsuite?
Attached file Simpler crasher
Here's a simplified page that also crashes Firefox, without getting any windows involved.
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)
With that patch applied, Firefox no longer crashes, but we do get bug 473630.  So there's more to be sorted out here.
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)
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
(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.
(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.
(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.
(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
(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
Whiteboard: [sg:critical?]
(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.)
(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.
Revised per Igor's suggestions.
Attachment #357022 - Attachment is obsolete: true
Attachment #357244 - Flags: review?(igor)
Attachment #357022 - Flags: review?(igor)
Attachment #357244 - Flags: review?(igor) → review+
I mark the bug as critical as it leads to double-free.
Whiteboard: [sg:critical?] → [sg:critical]
To Jim: can you land this to TraceMonkey?
(In reply to comment #20)
> To Jim: can you land this to TraceMonkey?

Certainly.
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/73f9f5e01fef
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey
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?
Attachment #384503 - Flags: review?(igor) → review+
Thanks, Igor!  A favor: I haven't got my SSH machinery updated for committing to CVS; could you land that patch for me?
(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 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+
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
Whiteboard: [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
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.
Does not affect 1.8.
Flags: wanted1.8.1.x-
Group: core-security
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
Crash Signature: [@ JS_HashTableRawLookup]
You need to log in before you can comment on or make changes to this bug.