473075 - TM: HasProperty can still call into exotic lookupProperty hooks

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

Spun off from bug 466781 comment 16.

If a native resolve hook directly or indirectly examines the JS stack or global properties, or reenters the interpreter, then this is a bug.  No known cases yet, but some DOM resolve hooks are awfully hairy.

Comment 1

[Damon Sicore (:damons)]

Jason, can you give an opinion on whether this should block?  Andreas said there's a twist to this that we may not understand.  Thoughts?

Comment 2

[Jason Orendorff [:jorendorff]]

I don't know if this is important.  It is not a dup of bug 462027.

We could fix this independently of 462027 just by making HasProperty super paranoid and retrying from the interpreter if a resolve hook would be called.

Or we could fix this using the same imacro techniques as in bug 468782 and
bug 475761, both of which depend on the patch in 462027 landing.

Either way I suspect it's a small enough fix that we can mark it blocking just to be on the safe side.

Comment 3

[Damon Sicore (:damons)]

Thanks Jason.  Blocking P2.

Comment 4

[Brendan Eich [:brendan]]

Hey jst, besides document.all and other .all hacks, do we have resolve hooks on DOM prototype objects?

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Oh, you don't even need a resolve hook.

js> eval("y = <z/>;");
js> for (var x = 0; x < 3; ++x) { "" in y; }
Assertion failure: cx->bailExit, at ../jstracer.cpp:4628

#0  JS_Assert (s=0x1a54c2 "cx->bailExit"...)
#1  0x00122af7 in js_GetTopStackFrame at ../jstracer.cpp:4628
#2  0x0008b4e9 in js_LeaveTrace at jstracer.h:626
#3  0x0008f3ee in js_InternalInvoke at jsinterp.cpp:1383
#4  0x0009fbd6 in js_ConstructObject at ../jsobj.cpp:3170
#5  0x00104ad8 in ToXMLName at ../jsxml.cpp:3095
#6  0x001065f7 in xml_lookupProperty at ../jsxml.cpp:4955
#7  0x0018eeb2 in HasProperty at ../jsbuiltins.cpp:339
#8  0x0018ef76 in js_HasNamedProperty at ../jsbuiltins.cpp:353
#9  0x0027df9b in ?? ()
#10 0xbfffed38 in ?? ()
#11 0x00148571 in js_MonitorLoopEdge at ../jstracer.cpp:4270
#12 0x0006dee6 in js_Interpret at ../jsinterp.cpp:3712

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> Oh, you don't even need a resolve hook.
> 
> js> eval("y = <z/>;");
> js> for (var x = 0; x < 3; ++x) { "" in y; }
> Assertion failure: cx->bailExit, at ../jstracer.cpp:4628

Changing bugzilla fields as appropriate based on the discovery of this testcase.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2 says:
> We could fix this independently of 462027 just by making HasProperty super
> paranoid and retrying from the interpreter if a resolve hook would be called.
>
> Or we could fix this using the same imacro techniques as in bug 468782 and
> bug 475761, both of which depend on the patch in 462027 landing.

This bug's summary was wrong.  HasProperty is already sufficiently paranoid about resolve hooks.  But it does call non-native property lookup methods, which isn't safe.

The second approach is more work.  It doesn't seem justified yet.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 360725 [details] [diff] [review]
v1

>diff --git a/js/src/jsbuiltins.cpp b/js/src/jsbuiltins.cpp
>--- a/js/src/jsbuiltins.cpp
>+++ b/js/src/jsbuiltins.cpp
>@@ -323,25 +323,26 @@ js_AddProperty(JSContext* cx, JSObject* 
>     js_FreeSlot(cx, obj, slot);
>     JS_UNLOCK_SCOPE(cx, scope);
>     return JS_FALSE;
> }
> 
> static JSBool
> HasProperty(JSContext* cx, JSObject* obj, jsid id)
> {
>-    // check whether we know how the resolve op will behave
>+    // Check that we know how the lookup op will behave.
>     JSClass* clasp = OBJ_GET_CLASS(cx, obj);
>-    if (clasp->resolve != JS_ResolveStub && clasp != &js_StringClass)
>+    if (obj->map->ops->lookupProperty != js_LookupProperty ||
>+        (clasp->resolve != JS_ResolveStub && clasp != &js_StringClass)) {
>         return JSVAL_TO_BOOLEAN(JSVAL_VOID);
>+    }

Drive-by nit: not that it matters for perf since we'll need to clear both || tests, but it would be a bit more readable to split into two ifs and defer the clasp decl and init till after the first.

/be

Comment 10

[Andreas Gal :gal]

Comment on attachment 360725 [details] [diff] [review]
v1

Deferring to brendan. I don't know the resolution mechanism well enough I think.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 360725 [details] [diff] [review]
v1

Other than the nit this looks great. We still have the other bug on proto-resolve hooks, but this should do it for direct resolve/lookup mischief.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

note to self: js1_5/Regress/regress-271716-n.js; js1_5/Regress/regress-3649-n.js can fire the Assertion failure: cx->bailExit in 1.9.2 with jit enabled.

Comment 13

[Jason Orendorff [:jorendorff]]

    // Check that we know how the lookup op will behave.
    if (obj->map->ops->lookupProperty != js_LookupProperty)
        return JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID);
    JSClass* clasp = OBJ_GET_CLASS(cx, obj);
    if (clasp->resolve != JS_ResolveStub && clasp != &js_StringClass)
        return JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID);

http://hg.mozilla.org/tracemonkey/rev/54023cd2177c

Comment 14

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-3649-n.js still fires Assertion failure: cx->bailExit in tracemonkey.

filed bug 477351

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/54023cd2177c

Comment 16

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073

Comment 17

[Robert Sayre]

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

Comment 18

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2