src: create HandleScope for env retrieval by gabrielschulhof · Pull Request #30130 · nodejs/node · GitHub

Getting the current node::Environment using a
v8::Local<v8::Context> needs to be done within a v8::HandleScope.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

@gabrielschulhof Can you tell more about what this solves? The call would usually be inside a handle scope anyway because it takes a Local<> argument (the only exception I can think of would be if there was an additional SealHandleScope inside the outer HandleScope), and the part of the code that's being wrapped in a handle scope in this PR shouldn't allocate any new local handles (so if it does that might be a bug in V8)?

Edit: Looks like the slow path in https://github.com/v8/v8/blob/fbbcbba7bbb5c06031b74eef5ec895ceaaa91a97/src/api/api.cc#L1291-L1300 does need a HandleScope, yes, but preferably inside that method?

@addaleax in N-API we obtain the v8::Local<v8::Context> by converting a persistent reference to a local one: https://github.com/nodejs/node/blob/master/src/js_native_api_v8.h#L35 The conversion is just a cast (https://github.com/nodejs/node/blob/master/src/util.h#L717-L722), so we don't allocate any handle scopes. Thus, if the stack is such that it's coming straight from a weak callback then there is no handle scope in any of the frames leading up to it.

I agree that perhaps the V8 method should create a handle scope if it needs one and if there is any doubt as to whether a handle scope is present in any of the frames leading up to it.

@addaleax in N-API we obtain the v8::Local<v8::Context> by converting a persistent reference to a local one: https://github.com/nodejs/node/blob/master/src/js_native_api_v8.h#L35 The conversion is just a cast (https://github.com/nodejs/node/blob/master/src/util.h#L717-L722), so we don't allocate any handle scopes. Thus, if the stack is such that it's coming straight from a weak callback then there is no handle scope in any of the frames leading up to it.

I know, but … that’s kind of because we’re hacking into V8 internals here, and I’d argue that if we have to fix this in Node.js, the call site should behave as if it were accessing the handle properly (i.e. a HandleScope in node_napi_env__::node_env()).

I agree that perhaps the V8 method should create a handle scope if it needs one and if there is any doubt as to whether a handle scope is present in any of the frames leading up to it.

Ok – do you want to open a V8 CL or should I?

@addaleax actually, I think it may not be to V8 after all. The fact that we are hacking V8 here might allow the V8 folks to make the argument that what we're doing is not supported and therefore it's up to us (Node.js) to fix it.

But if it is up to us, then what is the best way? Does adding a handle scope come with a significant performance penalty? If so, we should add the HandleScope as far away from node::Environment::GetCurrent as possible so as to affect as few code paths as possible.

OTOH, if the performance penalty of adding a HandleScope is not so large, then adding the handle scope at napi_env__::context() makes the most sense because it would cover current and future use cases where a context is "somehow" (by way of our hack) available without first having created a HandleScope.

For perspective, I also ran into this problem as I was working on the reference cleanup in #28428.

NM, I don't think we can do napi_env__::context() because the HandleScope would immediately go out of scope 🤦

You can use an EscapableHandleScope and return handle_scope.Escape(context), but that relies on there being an outer handle scope.

Looking at the stack in #30127 I'm thinking that v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback might be a good place to declare a HandleScope because that's where we need the node::Environment for adding an immediate.

@addaleax actually, I think it may not be to V8 after all. The fact that we are hacking V8 here might allow the V8 folks to make the argument that what we're doing is not supported and therefore it's up to us (Node.js) to fix it.

I don’t think so. This is not the only way to trigger this issue (a SealHandleScope would also lead to it), and I think this qualifies as a handle leak otherwise.

Does adding a handle scope come with a significant performance penalty?

It’s not a huge difference, but it’s also avoidable and Environment::GetCurrent() is a frequently used function.

NM, I don't think we can do napi_env__::context() because the HandleScope would immediately go out of scope 🤦

It kind of doesn’t, as you pointed out – we don’t actually access the handle in a way that V8 doesn’t know anything about. But it also wouldn’t help with this issue, that’s true.

Looking at the stack in #30127 I'm thinking that v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback might be a good place to declare a HandleScope because that's where we need the node::Environment for adding an immediate.

I think that’s fine but if you do, maybe add a comment that that is a workaround and not intended to be permanent.

Fwiw, here’s a V8 CL for this: https://chromium-review.googlesource.com/c/v8/v8/+/1879902

@addaleax I updated the PR to add the HandleScope at v8impl::(anonymous namespace)::BufferFinalizer::FinalizeBufferCallback, and a comment to remove it if/once we have your CL.

Hi guys,

Is there an work around for this with existing 8, 10 and 12 releases?

Thanks

@shiretu I wouldn’t know how to work around this bug if you run into it, sorry.

That being said, I also wouldn’t know how to cause this bug to happen in a standard (non-debug) Node.js build – we don’t compile V8 with checks by default. Do you use the Node.js binary downloaded from https://nodejs.org/?

@shiretu we'll certainly backport this as far back as possible once it lands, but, of course, that'll take time.

@addaleax : yes, it happens with downloaded and locally compiled bins. The downloaded ones are fetched with ‘nvm’, but can’t say for sure where is it fetches them from.

@gabrielschulhof: thanks. I’ll watch this issue for closure than.

yes, it happens with downloaded and locally compiled bins.

That’s … somewhat concerning? SlowGetAlignedPointerFromEmbedderData should only really ever be called if N-API/Node.js was compiled with V8_ENABLE_CHECKS, but that is not the case for our release builds?

@shiretu Do you get the same issue with Node 13? Just to verify…

Also, the V8 CL has landed as v8/v8@e5dbc95cc0bf, so I think this PR can also be turned into a cherry-pick of that.

@addaleax: I have checked this again. Made absolutely sure that the right bins are used. 8, 10 and 12 from nvm all behave as expected, no crash.

The locally compiled version is produced like this:

./configure \
    --prefix=/tmp/node \
    --debug \
    --gdb \
    --without-intl \
    --ninja \

ninja -C out/Debug -j32

That is always failing.

I'm so sorry, I was so sure that the right bins were used in the tests, but it seems that I've made a mistake and I was running the locally compiled version thinking it was actually one of the official versions.

Nevertheless, the bug still stands IMHO: that finalizer is executed without a HS

@addaleax�: the locally compiled version is from a release tag, same version as the official releases

@shiretu Yeah, sure, a bug in debug mode is still a bug. But it’s good to know that this doesn’t affect release builds.

@cjihrig please take another look as the PR has changed radically.

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

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#64583} Refs: v8/v8@e5dbc95 Fixes: #30127 PR-URL: #30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Landed in 61d6144.

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#64583} Refs: v8/v8@e5dbc95 Fixes: nodejs#30127 PR-URL: nodejs#30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#64583} Refs: v8/v8@e5dbc95 Fixes: #30127 PR-URL: #30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#64583} Refs: v8/v8@e5dbc95 Fixes: nodejs#30127 PR-URL: nodejs#30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#64583} Refs: v8/v8@e5dbc95 Fixes: #30127 PR-URL: #30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#64583} Refs: v8/v8@e5dbc95 Fixes: nodejs#30127 PR-URL: nodejs#30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#64583} Refs: v8/v8@e5dbc95 Fixes: #30127 Backport-PR-URL: #30513 PR-URL: #30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#64583} Refs: v8/v8@e5dbc95 Fixes: nodejs#30127 PR-URL: nodejs#30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Fix handle leak when getting Context embedder data The `Context::SlowGetAlignedPointerFromEmbedderData()` method returns a pointer, so the fact that it allocates handles is not obvious to the caller. Since this is the slow path anyway, simply add a handle scope inside of it. The tests are also modified to perform the same check for the `Object` equivalent of this method. Change-Id: I5f03c9a7b70b3a17315609df021606a53c9feb2d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879902 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#64583} Refs: v8/v8@e5dbc95 Fixes: #30127 Backport-PR-URL: #30109 PR-URL: #30130 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gabrielschulhof requested a review from addaleax October 26, 2019 08:52

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 26, 2019

addaleax mentioned this pull request Oct 26, 2019

Finalizers are executed without a handle scope #30127

Closed

gabrielschulhof force-pushed the missing-handle-scope branch from 49bc1be to 3b52bc6 Compare October 26, 2019 18:49

addaleax previously approved these changes Oct 26, 2019

View reviewed changes

Comment thread src/node_api.cc Outdated

gabrielschulhof force-pushed the missing-handle-scope branch from 3b52bc6 to bf28635 Compare October 26, 2019 18:56

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Oct 26, 2019

cjihrig previously approved these changes Oct 26, 2019

View reviewed changes

gabrielschulhof force-pushed the missing-handle-scope branch from bf28635 to 383e888 Compare October 30, 2019 02:34

mhofman mentioned this pull request Oct 31, 2019

src: implement v8 host weakref hooks #29874

Merged

3 tasks

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 31, 2019

gabrielschulhof closed this Oct 31, 2019

gabrielschulhof deleted the missing-handle-scope branch October 31, 2019 20:59

targos mentioned this pull request Nov 5, 2019

v13.1.0 release proposal #30262

Merged

targos mentioned this pull request Nov 10, 2019

v12.13.1 release proposal #30352

Merged

BridgeAR mentioned this pull request Nov 21, 2019

v13.2.0 proposal #30547

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

src: create HandleScope for env retrieval by gabrielschulhof · Pull Request #30130 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: create HandleScope for env retrieval - #30130

src: create HandleScope for env retrieval#30130gabrielschulhof wants to merge 1 commit intonodejs:masternodejs/node:masterfrom gabrielschulhof:missing-handle-scopegabrielschulhof/node:missing-handle-scopeCopy head branch name to clipboard

Conversation

gabrielschulhof commented Oct 26, 2019

Checklist

Uh oh!

addaleax commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

addaleax commented Oct 26, 2019

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

devsnek commented Oct 26, 2019

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

addaleax commented Oct 26, 2019

Uh oh!

addaleax commented Oct 26, 2019

Uh oh!

gabrielschulhof commented Oct 26, 2019

Uh oh!

Uh oh!

shiretu commented Oct 27, 2019

Uh oh!

addaleax commented Oct 27, 2019

Uh oh!

gabrielschulhof commented Oct 27, 2019

Uh oh!

shiretu commented Oct 28, 2019

Uh oh!

addaleax commented Oct 28, 2019

Uh oh!

addaleax commented Oct 28, 2019

Uh oh!

shiretu commented Oct 28, 2019

Uh oh!

shiretu commented Oct 28, 2019

Uh oh!

addaleax commented Oct 28, 2019

Uh oh!

nodejs-github-bot commented Oct 31, 2019

Uh oh!

gabrielschulhof commented Oct 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src: create HandleScope for env retrieval#30130
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:missing-handle-scope

addaleax commented Oct 26, 2019 •
edited
Loading