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

NeedImmediateCallbackSetter terminates after setting global.process #17681

Closed
tniessen opened this issue Dec 14, 2017 · 4 comments
Closed

NeedImmediateCallbackSetter terminates after setting global.process #17681

tniessen opened this issue Dec 14, 2017 · 4 comments
Labels
process Issues and PRs related to the process subsystem.

Comments

@tniessen
Copy link
Member

  • Version: 8.9.3 (LTS)
  • Platform: Windows 10 Professional 64 bit
  • Subsystem: process

This causes node 8.9.3 to crash:

> global.process = { __proto__: global.process, pid: 123456 }
process { pid: 123456 }
> process._needImmediateCallback = true
true
> FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.

I came across this problem when executing unit tests of an npm package which intentionally replaces the process object. The problem seems to be caused by CheckImmediate which is called by NeedImmediateCallbackSetter via libuv:

node/src/node.cc

Lines 379 to 384 in ab46b8e

MakeCallback(env->isolate(),
env->process_object(),
env->immediate_callback_string(),
0,
nullptr,
{0, 0}).ToLocalChecked();

MakeCallback fails because callback_v is not a function:

node/src/node.cc

Lines 1528 to 1539 in ab46b8e

MaybeLocal<Value> MakeCallback(Isolate* isolate,
Local<Object> recv,
Local<String> symbol,
int argc,
Local<Value> argv[],
async_context asyncContext) {
Local<Value> callback_v = recv->Get(symbol);
if (callback_v.IsEmpty()) return Local<Value>();
if (!callback_v->IsFunction()) return Local<Value>();
Local<Function> callback = callback_v.As<Function>();
return MakeCallback(isolate, recv, callback, argc, argv, asyncContext);
}

node 9 is not affected.

@tniessen tniessen added process Issues and PRs related to the process subsystem. v8.x labels Dec 14, 2017
@targos
Copy link
Member

targos commented Dec 14, 2017

MakeCallback can return an empty Local, so CheckImmediate should check this instead of calling ToLocalChecked

@tniessen
Copy link
Member Author

cc @addaleax

@bnoordhuis
Copy link
Member

A better solution is to remove process._immediateCallback and introduce some internal API that can't be tampered with. Shouldn't be too hard, there's only place in lib/timers.js that uses it.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 18, 2017
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Fixes: nodejs#17681
@bnoordhuis
Copy link
Member

#17736

MylesBorins pushed a commit that referenced this issue Jan 10, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
apapirovski pushed a commit to apapirovski/node that referenced this issue Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: nodejs#17736
Fixes: nodejs#17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants