Don't use userobject.call/userobject.apply #12956

daurnimator · 2017-05-11T01:29:04Z

Version: v7.9.0

Internally node shouldn't rely on user provided functions/objects having 'normal' values.

e.g.

a = function() { console.log(1,2,3)};
a.call = "some value"
setTimeout(a, 100); true

Output:

TypeError: callback.call is not a function
    at ontimeout (timers.js:386:14)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)

For this example, node's timers.js should be using Function.prototype.call or Reflect.apply

The text was updated successfully, but these errors were encountered:

daurnimator · 2017-05-11T01:53:19Z

@mscdex this is not timers specific: this applies across many of node's libs (timers included).

another example (from util.js):

util = require("util");
a = function() {};
b = function() { return "b" }
a[util.inspect.custom] = b
b.call = "some value"
a

Output:

TypeError: maybeCustomInspect.call is not a function
    at formatValue (util.js:369:36)
    at Object.inspect (util.js:203:10)
    at REPLServer.self.writer (repl.js:459:19)
    at finish (repl.js:584:38)
    at REPLServer.defaultEval (repl.js:377:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:191:7)

and also in sameish area:

util = require("util");
a = function() {};
a[util.inspect.custom] = function() { return "a" }; 
Object.defineProperty(a, "constructor", {get: function() {throw Error()}})

Output:

Error
    at Function.get (repl:1:65)
    at formatValue (util.js:368:16)
    at Object.inspect (util.js:203:10)
    at REPLServer.self.writer (repl.js:459:19)
    at finish (repl.js:584:38)
    at REPLServer.defaultEval (repl.js:377:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)

refack · 2017-05-11T02:16:15Z

@daurnimator I believe that in both cases node adheres to the standards

setTimeout - https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-steps
Object.defineProperty - http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.6
Admittedly in the second case it throws because node called util.inspect.custom but that's only in the REPL. If you run it as a script, nothing will happen.
node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); 5"

Node implements JavaScript (more precisely ECMAscript), and like many other programing languages, it allows you to make mistakes, from never ending loops, to division by 0, and yes, even manipulating the internal mechanics to throw errors.

I'm going to close this issue, since I didn't find a deviation from the standards. If you find such a deviation, feel free to reopen, and give us an example so we can do better...

daurnimator · 2017-05-11T02:21:01Z

setTimeout - https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-steps

When the standard says "Invoke the Function" it doesn't mean to use .call, it means to use the internal mechanism for calling (which is exposed by Reflect.apply).
See https://heycam.github.io/webidl/#invoke-a-callback-function which leads to https://tc39.github.io/ecma262/#sec-call

Object.defineProperty - http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.6
Admittedly in the second case it throws because node called util.inspect.custom but that's only in the REPL. If you run it as a script, nothing will happen.
node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); 5"

Try printing/inspecting a:

$ node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); console.log(a)"
[eval]:1
var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); console.log(a)
                                                                                                                                     ^

Error
    at Function.get ([eval]:1:140)
    at formatValue (util.js:368:16)
    at inspect (util.js:203:10)
    at exports.format (util.js:69:24)
    at Console.log (console.js:43:37)
    at [eval]:1:160
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at Object.runInThisContext (vm.js:95:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:571:32)

Node implements JavaScript (more precisely ECMAscript), and like many other programing languages, it allows you to make mistakes, from never ending loops, to division by 0, and yes, even manipulating the internal mechanics to throw errors.

IMO internal mechanics throwing errors is an entirely different category to infinite loops and divide by zero.
I'll also note that this is an issue that popped up for me when trying to run+debug code under node.js; which ended up just making things more confusing.

I'm going to close this issue, since I didn't find a deviation from the standards. If you find such a deviation, feel free to reopen, and give us an example so we can do better...

Please see above: .call is not a standards compliant way to "invoke a function"

refack · 2017-05-11T02:41:27Z

@daurnimator note that in all cases IMHO node did what you asked it to do, and most importantly did not crash. It provided you with a proper Error with a stack trace.

We can argue the trade-offs in the implementation of the 'util' module (i.e. printing/inspecting) it is a node module that is not part of the standards, and indeed has it's limitations. For example Reflect.apply is a new language construct, and is not yet optimized for performance, so we did not refactor our code to use it (where appropriate).
We also try not to "break" the status-quo unless there is a very compelling reason, since what you consider a bug, other have come to rely upon as expected behaviour, and us changing that behaviour might bring more harm than good in the wider scope of the entire ecosystem.

Trott · 2017-05-11T02:46:45Z

I'm going to re-open this. I think @daurnimator has a point. In particular, the first sample code provided runs as expected in the browser (tested on Chrome) and I would think we'd want to emulate that robustness.

daurnimator · 2017-05-11T02:46:48Z

We can argue the trade-offs in the implementation of the 'util' module (i.e. printing/inspecting) it is a node module that is not part of the standards, and indeed has it's limitations.

Please note that this issue can be found all across the node code-base. I've mentioned setTimeout as the behaviour can be easily compared to browsers (and is standardised), and util as I haven't figured out a workaround when debugging.

Reflect.apply is a new language construct, and is not yet optimized for performance, so we did not refactor our code to use it (where appropriate).

Function.prototype.call would have been the correct choice beforeReflect was introduced.

refack · 2017-05-11T02:50:23Z

Also as you might know node welcomes all contributions. If you feel you can help us improve the util module or other in general, I invite to you open a PR.
I think a good start would be to add your examples as tests to our test suite (/test/parallel/) you can see some of the files in https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=util+extension%3Ajs+path%3A%2Ftest%2Fparallel%2F&type=

refack · 2017-05-11T02:58:45Z

I'm going to re-open this. I think @daurnimator has a point. In particular, the first sample code provided runs as expected in the browser (tested on Chrome) and I would think we'd want to emulate that robustness.

@Trott IMHO it's not a matter of robustness, since there was not crash, nor any non-deterministic behaviour. I obviously welcome the re-open If you feel we're deviating from the standard.

refack · 2017-05-11T03:10:06Z

Just tried this:

b = Object.create(Function)
setTimeout(b, 0)

Chrome:

Uncaught TypeError: Function.prototype.toString is not generic
    at Function.toString (<anonymous>)
    at <anonymous>:1:1
(anonymous) @ VM271:1

node

TypeError: "callback" argument must be a function
    at exports.setTimeout (timers.js:348:11)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:191:7)
    at REPLServer.Interface._onLine (readline.js:241:10)
    at REPLServer.Interface._line (readline.js:590:8)
    at REPLServer.Interface._ttyWrite (readline.js:869:14)

IMHO our Error is better. You win some, you lose some...

daurnimator · 2017-05-11T03:14:10Z

@refack No that is standard behaviour.
As your b object is not a function ((typeof Object.create(Function)) == "object") it tries to eval() it (which does the .toString, or to be more explicit, the Object.prototype.toString.call(yourobject))

refack · 2017-05-11T03:31:06Z

@daurnimator Ok so my reference to the WhatWG setTimeout standard was wrong.
Node has it's own implementation of setTimeout that doesn't eval() the first argument, and will only accept a function. https://nodejs.org/api/timers.html#timers_settimeout_callback_delay_args
But in this case node's error is more informative than Chrome's.

All I'm saying it's a matter of trade offs.
That, and show your enthusiasm with code; write some tests, or better yet, help improve the timers implementation. 👍

I'll finish with an open question
var a = Object.create(null)
Is a and object? Is a an Object?
On the one hand I can do

a['b'] = 1
1 === a.b

On the other hand

a instanceof Object === false

Working with a duck-typed language is subtle...

creationix · 2017-05-11T03:34:49Z

a = 5
a === 5
typeof a === 'number'
a instanceof Number === false

Yes Object.create(null) is an object, it's typeof is even 'object' (so is null, but that's another story).

Trott · 2017-05-11T04:06:42Z

Addressing just the timers part, not util or anything else: #12960

Timers should work even if the user has monkey-patched `.call()` and `.apply()` to undesirable values. Refs: nodejs#12956

refack · 2017-05-11T18:29:40Z

@daurnimator I followed up or your idea in #12981, you're invited to review.

Timers should work even if the user has monkey-patched `.call()` and `.apply()` to undesirable values. PR-URL: nodejs#12960 Ref: nodejs#12956 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Timers should work even if the user has monkey-patched `.call()` and `.apply()` to undesirable values. PR-URL: #12960 Ref: #12956 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Instead of callback bound apply, instead use the standard Reflect.apply. This is both safer and appears to offer a slight performance benefit. PR-URL: #17456 Refs: #12956 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Instead of callback bound apply, instead use the standard Reflect.apply. This is both safer and appears to offer a slight performance benefit. PR-URL: nodejs#17456 Refs: nodejs#12956 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Instead of callback bound apply, instead use the standard Reflect.apply. This is both safer and appears to offer a slight performance benefit. Backport-PR-URL: #18487 PR-URL: #17456 Refs: #12956 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Trott · 2018-03-04T01:01:18Z

@daurnimator In your estimation, should this remain open? If so, I imagine we should catalog all the instances and make a tracking issue so that we know when it will be closable. Although I wonder how long that list would be...

daurnimator · 2018-03-04T01:09:20Z

It appears we've fixed the most egregious cases (just see all the commits linked to this issue!). However I'm sure there's still a few we've missed.

I think we should close this for now; but perhaps it's something that can be added to code-review guidelines? i.e. "watch out for new code introduced that uses userobject.apply"

devsnek · 2018-03-04T01:16:51Z

@daurnimator progress is slowly being made, take a look at #17434, #18795 and #18773

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label May 11, 2017

refack added wontfix Issues that will not be fixed. and removed timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels May 11, 2017

refack closed this as completed May 11, 2017

Trott reopened this May 11, 2017

refack removed the wontfix Issues that will not be fixed. label May 11, 2017

refack added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label May 11, 2017

Trott mentioned this issue May 11, 2017

timers: do not use user object call/apply #12960

Closed

3 tasks

Trott added a commit to Trott/io.js that referenced this issue May 11, 2017

timers: do not use user object call/apply

4622913

Timers should work even if the user has monkey-patched `.call()` and `.apply()` to undesirable values. Refs: nodejs#12956

refack mentioned this issue Jun 8, 2017

util: add util.callbackify() #12712

Merged

4 tasks

refack mentioned this issue Sep 15, 2017

Possible fix for #15374 #15389

Closed

2 tasks

apapirovski mentioned this issue Nov 24, 2017

process: slightly simplify next tick execution #16888

Closed

2 tasks

apapirovski mentioned this issue Dec 4, 2017

events: use Reflect.apply #17456

Closed

2 tasks

starkwang mentioned this issue Dec 7, 2017

fs: use Reflect.apply #17514

Closed

2 tasks

Trott closed this as completed Mar 4, 2018

refack mentioned this issue Nov 23, 2018

Fixing child_process module to check values passed strictly to the options object #24267

Closed

4 tasks

joyeecheung mentioned this issue Sep 12, 2023

Removing primordials from Node.js project nodejs/TSC#1438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use userobject.call/userobject.apply #12956

Don't use userobject.call/userobject.apply #12956

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017

daurnimator commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017

Trott commented May 11, 2017

daurnimator commented May 11, 2017

refack commented May 11, 2017

refack commented May 11, 2017

refack commented May 11, 2017

daurnimator commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017 •

edited

Loading

creationix commented May 11, 2017 •

edited

Loading

Trott commented May 11, 2017

refack commented May 11, 2017

Trott commented Mar 4, 2018

daurnimator commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018

Don't use userobject.call/userobject.apply #12956

Don't use userobject.call/userobject.apply #12956

Comments

daurnimator commented May 11, 2017 • edited Loading

daurnimator commented May 11, 2017 • edited Loading

refack commented May 11, 2017

daurnimator commented May 11, 2017 • edited Loading

refack commented May 11, 2017

Trott commented May 11, 2017

daurnimator commented May 11, 2017

refack commented May 11, 2017

refack commented May 11, 2017

refack commented May 11, 2017

Chrome:

node

daurnimator commented May 11, 2017 • edited Loading

refack commented May 11, 2017 • edited Loading

creationix commented May 11, 2017 • edited Loading

Trott commented May 11, 2017

refack commented May 11, 2017

Trott commented Mar 4, 2018

daurnimator commented Mar 4, 2018 • edited Loading

devsnek commented Mar 4, 2018

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017 •

edited

Loading

creationix commented May 11, 2017 •

edited

Loading

daurnimator commented Mar 4, 2018 •

edited

Loading