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

Trott · 2017-05-11T04:05:58Z

setTimeout() and setInterval() should work even if the user has
monkey-patched .call() and .apply() to undesirable values. (This is
true for setImmediate() as well, but setImmediate() works just fine
in the current implementation. The test added here nonetheless adds a
test for setImmediate() as well as setTimeout() and setInterval().

Refs: #12956

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

timers

Trott · 2017-05-11T04:08:28Z

Will have to run a benchmark to see performance impact...

mscdex · 2017-05-11T04:09:26Z

~~Would be good to benchmark this as well.~~ You beat me to it!

Trott · 2017-05-11T04:10:20Z

Would be good to benchmark this as well.

Working on it...

Trott · 2017-05-11T04:10:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8003/

daurnimator · 2017-05-11T04:11:46Z

FWIW it used to be common with this sort of code to put at the top of the file:

var apply = (function(){}).apply;

And then use that (as it has no dependencies on even someone overriding the global Function)

daurnimator · 2017-05-11T04:12:29Z

lib/timers.js

        break;
      case 3:
-        callback.call(timer, args[0], args[1], args[2]);
+        Function.prototype.call.call(callback, timer,
+                                     args[0], args[1], args[2]);
        break;
      default:
        callback.apply(timer, args);


Missed the .apply case.

Make sure to add a test with >3 arguments.

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

daurnimator · 2017-05-11T06:17:30Z

Looks good now :)

Trott · 2017-05-11T06:21:05Z

Had to alter the implementation a bit from the initial attempt to get the benchmark to be neutral. Current implementation in this PR compares like this against master:

                                                    improvement confidence    p.value
 timers/immediate.js type="breadth" thousands=2000      -0.43 %            0.67056272
 timers/immediate.js type="breadth1" thousands=2000      0.10 %            0.92761906
 timers/immediate.js type="breadth4" thousands=2000     -1.59 %            0.19085909
 timers/immediate.js type="clear" thousands=2000         0.11 %            0.89212673
 timers/immediate.js type="depth" thousands=2000        -1.63 %            0.07178678
 timers/immediate.js type="depth1" thousands=2000        0.90 %            0.35150859
 timers/set-immediate-breadth-args.js millions=5         0.49 %            0.50576472
 timers/set-immediate-breadth.js millions=10            -0.34 %            0.36237802
 timers/set-immediate-depth-args.js millions=10         -1.44 %            0.10138701
 timers/set-immediate-depth.js millions=10              -0.81 %            0.42620317
 timers/timers-breadth.js thousands=500                 -0.18 %            0.80576168
 timers/timers-cancel-pooled.js thousands=500            1.10 %            0.27800863
 timers/timers-cancel-unpooled.js thousands=100          0.31 %            0.22503088
 timers/timers-depth.js thousands=1                      0.02 %            0.95753131
 timers/timers-insert-pooled.js thousands=500           -0.21 %            0.85337983
 timers/timers-insert-unpooled.js thousands=100          0.06 %            0.93189629
 timers/timers-timeout-pooled.js thousands=500          -0.77 %            0.48345237

Trott · 2017-05-11T06:22:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8004/

TimothyGu · 2017-05-11T08:36:52Z

Well, for perfect robustness (and Web IDL compliance, which I know we don't particularly care for) we need to get the initial value of Function.prototype.apply and cache it – and do this for every single built-in prototype method we use. I'm good with this change, but it's important to recognize that these type of changes could become slippery slopes very quickly.

refack · 2017-05-11T11:56:51Z

we need to get the initial value of Function.prototype.apply and cache it

I think we should leak C++ helpers for call and apply rather then cache the JS aliases. So we actually use "the [[Call]] internal method of a function object"

TimothyGu · 2017-05-11T15:03:15Z

I think we should leak C++ helpers for call and apply rather then cache the JS aliases. So we actually use "the [[Call]] internal method of a function object"

They are equivalent, and the C++ helpers will be much slower than JS.

Fishrock123

LGTM pending some benchmark / profiling output, which is the reason this code was this way.

Trott · 2017-05-11T17:45:48Z

LGTM pending some benchmark / profiling output, which is the reason this code was this way.

Benchmark results are included above. They show no significant performance change. No changes have been applied to this PR since those benchmarks have been run.

refack · 2017-05-11T17:52:06Z

They are equivalent, and the C++ helpers will be much slower than JS.

How about hiding the defaults behind symbols on Function during bootstrapping?

mscdex · 2017-05-11T17:54:14Z

The benchmark results look ok to me, but I am also concerned about this being a slippery slope. Just how far down the rabbit hole of protecting against userland do we have to go?

Trott · 2017-05-11T18:03:54Z

Just how far down the rabbit hole of protecting against userland do we have to go?

I'd like us to go this far down the rabbit hole:

Does the problematic behavior affect actual users or is it likely to affect actual users?
Is there a fix that does not have significant performance impact or that is likely to introduce more bugs?

If the answer to both questions is "yes" (or at least "seems likely"), then I'm good with it.

On the "affect actual users", I should note that I may be making an incorrect assumption here. I assumed that because @daurnimator opened an issue about this behavior, it is something that affected them.

refack · 2017-05-11T18:28:14Z

Well, for perfect robustness (and Web IDL compliance, which I know we don't particularly care for) we need to get the initial value of Function.prototype.apply and cache it

@TimothyGu I followed up on your suggestion #12981

jasnell · 2017-05-11T21:48:05Z

Just noting... I've been stewing over an internal module that captures the original exports for key items like toString(), apply(), call() etc on startup before any user code is able to run, specifically to protect ourselves from cases like this.

daurnimator · 2017-05-12T01:13:37Z

I should note that I may be making an incorrect assumption here. I assumed that because @daurnimator opened an issue about this behavior, it is something that affected them.

Indeed. And this was just the first thing I ran into, as setTimeout was the first function I called to test out some code. => The second function I called was console.log (and hence util.inspect) when this didn't work!
I expect I'll run into case after case of similar things to this as I work on my current project. I will continue to report things as I run into them.

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 <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2017-05-13T05:19:26Z

Landed in 98609fc.

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 <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins · 2017-06-22T18:45:54Z

is this applicable to lts v6.x?

Trott · 2017-06-23T05:19:49Z

is this applicable to lts v6.x?

@MylesBorins Yes.

MylesBorins · 2017-07-17T14:28:47Z

Failures on v6.x

== release test-timers-user-call ===
Path: parallel/test-timers-user-call
/Users/mborins/code/node/v6.x/test/common/index.js:440
    return fn.apply(this, arguments);
              ^

TypeError: fn.apply is not a function
    at Timeout._onTimeout (/Users/mborins/code/node/v6.x/test/common/index.js:440:15)
    at ontimeout (timers.js:386:11)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)
Command: out/Release/node /Users/mborins/code/node/v6.x/test/parallel/test-timers-user-call.js
[01:53|% 100|+ 1355|-   1]: Done
make: *** [test] Error 1

Can you please backport

Trott · 2017-07-17T14:57:16Z

@MylesBorins Needs #12027 to land first, then that error should go away.

Trott · 2017-07-17T14:58:00Z

(Removing backport-requested-v6.x because I don't think a backport is needed. But if I'm wrong, by all means, re-add it.)

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 <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins · 2017-08-14T17:48:51Z

@Trott I manually backported #12027 and this lands cleanly now!

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 <refack@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

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

Trott mentioned this pull request May 11, 2017

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

Closed

daurnimator reviewed May 11, 2017

View reviewed changes

Trott force-pushed the call branch 5 times, most recently from 5ab342a to aeb5af9 Compare May 11, 2017 05:07

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

Trott force-pushed the call branch from aeb5af9 to 4622913 Compare May 11, 2017 05:07

refack approved these changes May 11, 2017

View reviewed changes

Fishrock123 requested a review from mscdex May 11, 2017 15:11

Fishrock123 approved these changes May 11, 2017

View reviewed changes

joyeecheung approved these changes May 12, 2017

View reviewed changes

Trott closed this May 13, 2017

refack mentioned this pull request May 13, 2017

util,console: guard against overwritten util functions #13011

Closed

3 tasks

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jun 22, 2017

MylesBorins added the backport-requested-v6.x label Jul 17, 2017

Trott removed the backport-requested-v6.x label Jul 17, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Trott deleted the call branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Trott commented May 11, 2017

Trott commented May 11, 2017

mscdex commented May 11, 2017 •

edited

Loading

Trott commented May 11, 2017

Trott commented May 11, 2017

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017

Trott commented May 11, 2017

Trott commented May 11, 2017

TimothyGu commented May 11, 2017

refack commented May 11, 2017

TimothyGu commented May 11, 2017

Fishrock123 left a comment

Trott commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017

mscdex commented May 11, 2017

Trott commented May 11, 2017 •

edited

Loading

refack commented May 11, 2017

jasnell commented May 11, 2017

daurnimator commented May 12, 2017

Trott commented May 13, 2017

MylesBorins commented Jun 22, 2017

Trott commented Jun 23, 2017

MylesBorins commented Jul 17, 2017

Trott commented Jul 17, 2017

Trott commented Jul 17, 2017

MylesBorins commented Aug 14, 2017

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

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

Conversation

Trott commented May 11, 2017

Checklist

Affected core subsystem(s)

Trott commented May 11, 2017

mscdex commented May 11, 2017 • edited Loading

Trott commented May 11, 2017

Trott commented May 11, 2017

daurnimator commented May 11, 2017 • edited Loading

daurnimator May 11, 2017 • edited Loading

Choose a reason for hiding this comment

daurnimator commented May 11, 2017

Trott commented May 11, 2017

Trott commented May 11, 2017

TimothyGu commented May 11, 2017

refack commented May 11, 2017

TimothyGu commented May 11, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Trott commented May 11, 2017 • edited Loading

refack commented May 11, 2017

mscdex commented May 11, 2017

Trott commented May 11, 2017 • edited Loading

refack commented May 11, 2017

jasnell commented May 11, 2017

daurnimator commented May 12, 2017

Trott commented May 13, 2017

MylesBorins commented Jun 22, 2017

Trott commented Jun 23, 2017

MylesBorins commented Jul 17, 2017

Trott commented Jul 17, 2017

Trott commented Jul 17, 2017

MylesBorins commented Aug 14, 2017

mscdex commented May 11, 2017 •

edited

Loading

daurnimator commented May 11, 2017 •

edited

Loading

daurnimator May 11, 2017 •

edited

Loading

Trott commented May 11, 2017 •

edited

Loading

Trott commented May 11, 2017 •

edited

Loading