Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Illegal invocation error using ES6 Proxy and node.js #11629

Closed
FranckFreiburger opened this issue Mar 1, 2017 · 17 comments
Closed

Illegal invocation error using ES6 Proxy and node.js #11629

FranckFreiburger opened this issue Mar 1, 2017 · 17 comments
Labels
question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.

Comments

@FranckFreiburger
Copy link

Hello,
I am unable to call a node.js native function through a No-op forwarding proxy.

The following case works properly with a v8 native function:

var p = new Proxy(Math, {});
console.log( p.cos.toString() ); // function cos() { [native code] }
console.log( p.cos(0) ); // 1

Whereas the following case throw TypeError: Illegal invocation

var p = new Proxy(require('os'), {});
console.log( p.cpus.toString() ); // function getCPUs() { [native code] }
console.log( p.cpus() ); // TypeError: Illegal invocation

what's wrong ?

@mscdex mscdex added question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency. labels Mar 1, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 1, 2017

Maybe it is because the Math methods are static, while the Node.js modules' methods could require an appropriate receiver. This works:

const os = require('os');
const p = new Proxy(os, {});
console.log(p.cpus);
console.log(p.cpus.call(os));

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 1, 2017

@FranckFreiburger
Copy link
Author

receiver for the default get proxy handler is handled with Reflect.get(target, property, receiver)

@FranckFreiburger
Copy link
Author

This case is also weird (note that I use node.js v7.7.1):

const p = new Proxy(require('os'), {});

var cpus = p.cpus;
console.log(cpus()); // ok

console.log(p.cpus()); // Illegal invocation

@vsemozhetbyt
Copy link
Contributor

@FranckFreiburger
Maybe this is somehow connected with C++ bindings, This works:

var p = new Proxy(require('os'), {});
console.log(p.arch.toString());
console.log(p.arch());

os.arch() does not use bindings. os.cpus() does use them.

@TimothyGu
Copy link
Member

This issue isn't reproducible on master or v7.x-staging, probably as a side effect of #11564. Either way, the next release of v7.x will have this fixed.

@TimothyGu
Copy link
Member

TimothyGu commented Mar 6, 2017

cpus is only one function affected by this bug. Other methods that directly call C++ bindings are also vulnerable. On the V8 level, it is probably because the API call helper cannot find a compatible receiver in a Proxy object.

This bug has already been reported upstream by as issue 5773.

/cc @nodejs/v8

@FranckFreiburger
Copy link
Author

FranckFreiburger commented Mar 6, 2017

Note that all binding.* functions seems to be affected by this bug.

new Proxy(require('os'), {}).hostname() // Illegal invocation ...

@Trott
Copy link
Member

Trott commented Jul 30, 2017

Is there anything Node.js can/should do about this other than await https://bugs.chromium.org/p/v8/issues/detail?id=5773 getting fixed in V8? Should this issue remain open?

@targos
Copy link
Member

targos commented Aug 3, 2017

This is still an issue but AFAICT there is nothing Node.js can do about it.

Repro on v8.x:

const binding = process.binding('util');
const isDate = binding.isDate;
console.log(isDate(new Date)); // true
console.log(isDate.call(binding, new Date)); // true
console.log(isDate.call(new Proxy(binding, {}), new Date)); // illegal invocation

@mappum
Copy link

mappum commented Mar 11, 2018

This is still an issue as of Node v9.8.0. Can/should anyone push for a v8 fix, or should there be any sort of Node workaround? Or are users expected to rebind to the unproxied contexts for all of these calls?

@mappum
Copy link

mappum commented Mar 12, 2018

BTW, this comes up in surprisingly common places, e.g. trying to stringify a Proxied Buffer.

let buf = new Proxy(Buffer.from('test'), { get: (t, n) => t[n] })
console.log(buf)

@devsnek
Copy link
Member

devsnek commented Mar 12, 2018

fwiw this is intended behavior on the side of proxies. check out https://github.com/nodejs/node/pull/18131/files#diff-cc63475f116262e9cbea02374e344c01R671 for an example of a proxy that covers this use case.

@mappum
Copy link

mappum commented Mar 12, 2018

Even if this is fully compliant with the spec, it's quite a pain. As Proxies become a more widely adopted feature I expect many more people will run into this. At the very least, there should be a workaround on the Node side.

@devsnek
Copy link
Member

devsnek commented Mar 12, 2018

@mappum that's the general idea of the membrane concept: https://www.npmjs.com/package/es7-membrane

@bnoordhuis
Copy link
Member

The examples listed in this issue work with master so I think this can be closed out.

As to this example (edited for brevity):

new Proxy(Buffer.from('test'), { get: (t, n) => t[n] }).toString()

That invokes Buffer.prototype.toString() with the proxy as the receiver and fails the way it is expected to.

Talk of "there should be a workaround on the Node side" is misguided because it's not a sensible construct. Write it like this and note the .bind() call:

new Proxy(Buffer.from('test'), { get: (t, n) => t[n].bind(t) }).toString()

@kamil-kielczewski
Copy link

kamil-kielczewski commented May 31, 2023

On chrome I have same problem for events (eg. click event) - here is solution

const eventProxy = new Proxy(event, {
    get: (target, prop) => ()=> target[prop].apply(target),
});

working editable example here which shows how to add your code before/after original function execution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants