Don't use userobject.call/userobject.apply

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

@mscdex

@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)

@daurnimator

@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...

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"

@daurnimator

@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.

@daurnimator

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.

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.

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=

@daurnimator

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.

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...

timers: do not use user object call/apply

events: use Reflect.apply

@daurnimator

@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...

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"

@daurnimator

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

Navigation Menu

Repository navigation

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

Chrome:

node

13 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

daurnimator commented on May 11, 2017

refack commented on May 11, 2017

daurnimator commented on May 11, 2017

refack commented on May 11, 2017

Trott commented on May 11, 2017

daurnimator commented on May 11, 2017

refack commented on May 11, 2017

refack commented on May 11, 2017

refack commented on May 11, 2017

Chrome:

node

13 remaining items

Trott commented on Mar 4, 2018

daurnimator commented on Mar 4, 2018

devsnek commented on Mar 4, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions