Closed Bug 1138157 Opened 5 years ago Closed 5 years ago

Crash: Proxy revoked after trap extracted

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: anba, Assigned: evilpie)

References

Details

Attachments

(2 files)

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`
Thanks, André. I definitely had enough of these bugs now. I will see if we can stop inheriting from DirectProxyHandler.
Assignee: nobody → evilpies
Component: JavaScript Engine → JavaScript: Standard Library
Blocks: 978228
Why should the last one throw?
(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
Indeed, thanks.
Attached patch v1 - TestSplinter Review
André can I use your test here?
Attachment #8576920 - Flags: review?(efaustbmo)
Attachment #8576920 - Flags: feedback?(andrebargull)
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
Attachment #8576920 - Flags: feedback?(andrebargull) → feedback+
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.
Attachment #8576915 - Flags: review?(efaustbmo) → review+
Attachment #8576920 - Flags: review?(efaustbmo) → review+
I guess we don't really need weakmapKeyDelegate, because we can't unwrap a ScriptedDirectProxy.
https://hg.mozilla.org/mozilla-central/rev/2cf8496afd5d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.