454704 - Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

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)
...

Comment 1

[Jim Blandy :jimb]

I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.

Comment 2

[Jim Blandy :jimb]

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)

Comment 3

[Robert Sayre]

Why is XPCSafeJSObjectWrapper is accessible to content?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Historical reasons.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(Namely, when it was originally implemented, it was available. I don't think there's a specific reason, though.)

Comment 6

[Jim Blandy :jimb]

Attachment: Simpler crasher

Here's a simplified page that also crashes Firefox, without getting any windows involved.

Comment 7

[Jim Blandy :jimb]

Attachment: Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

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?)

Comment 8

[Jim Blandy :jimb]

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

[Brendan Eich [:brendan]]

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

Comment 10

[Brendan Eich [:brendan]]

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

[Igor Bukanov]

(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.

Comment 12

[Jim Blandy :jimb]

(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.

Comment 13

[Jim Blandy :jimb]

(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

[Brendan Eich [:brendan]]

(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

[Igor Bukanov]

(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.

Comment 16

[Jim Blandy :jimb]

(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

[Igor Bukanov]

(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.

Comment 18

[Jim Blandy :jimb]

Attachment: Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

Revised per Igor's suggestions.

Comment 19

[Igor Bukanov]

I mark the bug as critical as it leads to double-free.

Comment 20

[Igor Bukanov]

To Jim: can you land this to TraceMonkey?

Comment 21

[Jim Blandy :jimb]

(In reply to comment #20)
> To Jim: can you land this to TraceMonkey?

Certainly.

Comment 22

[Jim Blandy :jimb]

Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/73f9f5e01fef

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-454704.js

Comment 25

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/da90ad6653c2

Comment 26

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 27

[Jim Blandy :jimb]

Attachment: Bug 464704: Adapt fix for 1.9.0 tree.

Adapt fix for 1.9.0 tree.  Verified that Firefox crashes without patch, doesn't crash with.

Comment 28

[Jim Blandy :jimb]

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

[Igor Bukanov]

(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

[Samuel Sidler (old account; do not CC)]

Comment on attachment 384503 [details] [diff] [review]
Bug 464704: Adapt fix for 1.9.0 tree.


Approved for 1.9.0.12. a=ss

Comment 31

[Igor Bukanov]

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

Comment 32

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Does not affect 1.8.

Comment 34

[Bob Clary [:bc] (inactive)]

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