Closed Bug 473075 Opened 11 years ago Closed 11 years ago

TM: HasProperty can still call into exotic lookupProperty hooks

Categories

(Core :: JavaScript Engine, defect, P2, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(4 keywords, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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.
Flags: blocking1.9.1?
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?
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.
Thanks Jason.  Blocking P2.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Hey jst, besides document.all and other .all hacks, do we have resolve hooks on DOM prototype objects?

/be
Duplicate of this bug: 477049
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
Assignee: general → jorendorff
(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.
Severity: normal → critical
Version: Other Branch → Trunk
Attached patch v1Splinter Review
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.
Attachment #360725 - Flags: review?(gal)
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
Attachment #360725 - Flags: review?(gal) → review?(brendan)
Comment on attachment 360725 [details] [diff] [review]
v1

Deferring to brendan. I don't know the resolution mechanism well enough I think.
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
Attachment #360725 - Flags: review?(brendan) → review+
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.
    // 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
Summary: TM: HasProperty can still call into native resolve hooks → TM: HasProperty can still call into exotic lookupProperty hooks
Whiteboard: fixed-in-tracemonkey
js1_5/Regress/regress-3649-n.js still fires Assertion failure: cx->bailExit in tracemonkey.

filed bug 477351
http://hg.mozilla.org/mozilla-central/rev/54023cd2177c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073
Flags: in-testsuite+
Flags: in-litmus-
js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.