Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
critical
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: jimb)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
crash, testcase, verified1.9.0.12, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.8.1.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey, crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 337991 [details]
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)
...

Updated

9 years ago
Flags: blocking1.9.1+
(Assignee)

Updated

9 years ago
Assignee: general → jim
(Assignee)

Comment 1

9 years ago
I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.
(Assignee)

Comment 2

9 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

9 years ago
Priority: -- → P2

Comment 3

9 years ago
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.)
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Comment 6

9 years ago
Created attachment 357021 [details]
Simpler crasher

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

Comment 7

9 years ago
Created attachment 357022 [details] [diff] [review]
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?)
Attachment #357022 - Flags: review?(sayrer)
(Assignee)

Comment 8

9 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 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

Comment 11

9 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

9 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

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

9 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
Whiteboard: [sg:critical?]
(Assignee)

Comment 16

9 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

9 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

9 years ago
Created attachment 357244 [details] [diff] [review]
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

Revised per Igor's suggestions.
Attachment #357022 - Attachment is obsolete: true
Attachment #357244 - Flags: review?(igor)
Attachment #357022 - Flags: review?(igor)

Updated

9 years ago
Attachment #357244 - Flags: review?(igor) → review+

Comment 19

9 years ago
I mark the bug as critical as it leads to double-free.
Whiteboard: [sg:critical?] → [sg:critical]

Comment 20

9 years ago
To Jim: can you land this to TraceMonkey?
(Assignee)

Comment 21

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

Certainly.
(Assignee)

Comment 22

9 years ago
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/73f9f5e01fef
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 24

9 years ago
Created attachment 358055 [details]
js1_5/extensions/regress-454704.js

Updated

9 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-

Comment 25

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/da90ad6653c2
Keywords: fixed1.9.1

Comment 26

9 years ago
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1

Updated

8 years ago
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
(Assignee)

Comment 27

8 years ago
Created attachment 384503 [details] [diff] [review]
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.
Attachment #384503 - Flags: review?(igor)
Attachment #384503 - Flags: approval1.9.0.12?

Updated

8 years ago
Attachment #384503 - Flags: review?(igor) → review+
(Assignee)

Comment 28

8 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

8 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 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

8 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

8 years ago
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.
Keywords: fixed1.9.0.12 → verified1.9.0.12

Comment 33

8 years ago
Does not affect 1.8.
Flags: wanted1.8.1.x-
Group: core-security

Comment 34

8 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
Crash Signature: [@ JS_HashTableRawLookup]
You need to log in before you can comment on or make changes to this bug.