IsArray should throw when called on revoked proxy

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: anba, Assigned: Waldo)

Tracking

(Depends on: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Test case:
---
var {proxy, revoke} = Proxy.revocable([], {});
revoke();
Array.isArray(proxy);
---

Expected: Throws TypeError
Actual: Returns false

Spec: ES2015, 7.2.2 IsArray, step 3.a
Related: bug 1111785
I have most of a patch for this, but it involves adding a getUnsafeTarget proxy hook.  That's perfectly doable, but our current system of hooking through the vtable means that I'm unsure my patch overrides the hook in *every* class that should override it.  In bug 1198056 I'm going to make the proxy vtable into an explicit struct of functions so that I'm 100% confident I've correctly defined the new hook in all proxy handlers.  Then I'll circle back to this.
Assignee: nobody → jwalden+bmo
Depends on: 1198056
Created attachment 8654459 [details] [diff] [review]
Patch

Because the IsArray method does its own looping, without delegating to anything else, any error that's thrown *must* be from Array.isArray's compartment.  So we need to do an "unsafe" loop, accessing cross-compartment pointers while being careful not to do anything dangerous with them.  The existing objectClassIs trap (previously used to implement this) does this, so this is not quite the worst thing in the world.  :-\

There's some cargo-culting of definitions for some of these handlers (particularly the IPC ones), based on copying existing traps with similar-ish semantics.  Feel free to punt review on any particular part to someone more knowledgeable.

Haven't tryservered this yet, will certainly do so before landing.  I don't expect it would reveal any fundamental issues, so it doesn't seem necessary to have before reviewing might start -- any issues would be small.

How are you supposed to know I've touched all the right places?  First, know that this touched every place where an objectClassIs operation was specified/implemented.  Since IsArray used objectClassIs before, this can be no worse (along that axis) than the prior state.  Second, I have a followup patch directly atop this that renames objectClassIs to getBuiltinClass and modifies its signature, that I'll be posting momentarily to bug 1179003.  That pointed me at all the relevant places to change.  Third, the combination of the two -- which I'll post after posting that patch -- clearly changes *both* sibling traps each and every time it makes a change.  That should be sufficient to verify that all necessary additions were made here (and all changes made in bug 1179003).
Attachment #8654459 - Flags: review?(efaustbmo)
Blocks: 1179003
Depends on: 1199887
Created attachment 8657967 [details] [diff] [review]
Patch, v2

The tactic in the previous patch won't work.  The JS::IsArray method in it *only* can indicate an object's an array, if an actual, honest-to-goodness array (or unboxed facsimile) is found as a target.  But not all arrays will occur this way -- particularly not CPOWs.  That issue's fixable, but only if you add another proxy trap.  And at that point, it might as well be a full isArray trap.

So a full trap it is.  I take a somewhat interesting approach here: rather than indicating array-or-not via outparam, I indicate array/not-an-array/revoked-proxy (with the further possibility of the trap failing and throwing an exception).  This preserves the spec requirement of throwing a TypeError on a revoked proxy from another compartment.  And for CPOWs specifically, in mind because of the issue above, we don't have the CPOW throwing an exception that has to cross a process boundary and *definitely* not be from Array.isArray's compartment.  There's a little fun to marshall JS::IsArrayAnswer (an enum class) as a uint32_t across the CPOW boundary, but nothing complicated.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c41cc768f2b5 is colorful, but everything there does seem to be unrelated, so this should be good to review now.
Attachment #8654459 - Attachment is obsolete: true
Attachment #8654459 - Flags: review?(efaustbmo)
Attachment #8657967 - Flags: review?(efaustbmo)

Comment 4

2 years ago
Comment on attachment 8657967 [details] [diff] [review]
Patch, v2

Review of attachment 8657967 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

I guess we have CPOW tests already. I still don't fully understand why we can't have CPOWs forward errors fromthe target process to the host process, and just do this directly. as CCWs do. If they just pass-by-wrapper, then they do, and we have to do this.

::: js/src/json.cpp
@@ +532,4 @@
>      RootedObject obj(cx, &v.toObject());
>  
>      scx->depth++;
> +    auto dec = mozilla::MakeScopeExit([&] { scx->depth--; });

That's a lot of machinery to avoid putting a scx->depth-- in the !IsArray case, but I guess it's more robust in terms of further changes.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +1105,4 @@
>  ScriptedDirectProxyHandler::objectClassIs(HandleObject proxy, ESClassValue classValue,
>                                            JSContext* cx) const
>  {
> +    return false;

MOZ_ASSERT(classValue != ESClass_Array) or something?
Attachment #8657967 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/6c90d3eab1f7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.