1138157 - Crash: Proxy revoked after trap extracted

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect, critical)

Description

[André Bargull [:anba]]

The default implementations in DirectProxyHandler access `proxy->as<ProxyObject>().target()`, but side-effects may already have revoked the proxy. This leads to null-pointer deref in multiple places.

function p(o = {}) {
  var {proxy, revoke} = Proxy.revocable(o, new Proxy({}, {
    get(t, pk, r) {
      print("get:", pk);
      revoke();
    }
  }));
  return proxy;
}


Crash:

Object.isExtensible(p());
Object.preventExtensions(p());
Object.getOwnPropertyDescriptor(p(), "a");
Object.defineProperty(p(), "a", {});
"a" in p();
p().a;
p().a = 0;
delete p().a;
for (k in p()) ;
Object.getOwnPropertyNames(p());


Incorrect behaviour:

js> p(function() { return "ok" })()

Expected: Returns "ok"
Actual: Throws `TypeError: p(...) is not a function`

js> new (p(function(){}))

Expected: Throws `TypeError: illegal operation attempted on a revoked proxy`
Actual: Throws `TypeError: p(...) is not a constructor`

Comment 1

[Tom Schuster [:evilpie]]

Thanks, André. I definitely had enough of these bugs now. I will see if we can stop inheriting from DirectProxyHandler.

Comment 2

[Tom Schuster [:evilpie]]

Why should the last one throw?

Comment 3

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #2)
> Why should the last one throw?

The proxy should throw a TypeError when GetPrototypeFromConstructor is called. It's quite possible this part of the ES6 spec isn't yet implemented in SM.

The relevant spec steps:
9.5.14 [[Construct]] ( argumentsList, newTarget), step 7.b
-> 7.3.14 Construct (F, [argumentsList], [newTarget]), step 5
-> 9.2.3 [[Construct]], step 5.a
-> 9.1.14 OrdinaryCreateFromConstructor, step 2
-> 9.1.15 GetPrototypeFromConstructor ( constructor, intrinsicDefaultProto ), step 3
-> 9.5.8 [[Get]] (P, Receiver), step 3
-> Access on revoked proxy

Comment 4

[Tom Schuster [:evilpie]]

Indeed, thanks.

Comment 5

[Tom Schuster [:evilpie]]

Attachment: v1 - Change ScriptedDirectProxy to inherit directly from BaseProxy

Comment 6

[Tom Schuster [:evilpie]]

Attachment: v1 - Test

André can I use your test here?

Comment 7

[André Bargull [:anba]]

Comment on attachment 8576920 [details] [diff] [review]
v1 - Test

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

(In reply to Tom Schuster [:evilpie] from comment #6)
> Created attachment 8576920 [details] [diff] [review]
> v1 - Test
> 
> André can I use your test here?

Sure, why not. It may make sense to add tests for the currently missing traps (getPrototypeOf and setPrototypeOf). And also check things work as expected if a property is already present resp. not present. For example like: https://gist.github.com/anba/dcb1f739fd2137f73cbe

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8576915 [details] [diff] [review]
v1 - Change ScriptedDirectProxy to inherit directly from BaseProxy

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

Yep. This will silence the noise. r=me

The new weakmap function needs tests.

::: js/src/proxy/ScriptedDirectProxyHandler.h
@@ +16,3 @@
>    public:
>      MOZ_CONSTEXPR ScriptedDirectProxyHandler()
> +      : BaseProxyHandler(&family, false, false)

nit: aren't these two bollean params default? Either that, or comment what |false| means in each case.

Comment 9

[Tom Schuster [:evilpie]]

I guess we don't really need weakmapKeyDelegate, because we can't unwrap a ScriptedDirectProxy.

Comment 10

[Tom Schuster [:evilpie]]

Thanks again for your test André.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a9acbe6fde
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf8496afd5d

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2cf8496afd5d