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.

Sign up for GitHub

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

Jump to bottom

src: fix async hooks crashing when there is no node context #19134

Closed
wants to merge 42 commits into from
Closed

src: fix async hooks crashing when there is no node context #19134

Changes from 3 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
6b76017
src: fix async hooks crashing when there is no node context
xaviergonz Mar 4, 2018
84a1822
src: optimization for check for env promise hook
xaviergonz Mar 4, 2018
5e9128f
src: fixed linting errors for env.cc
xaviergonz Mar 4, 2018
9690b86
fix: updated with the latest code review
xaviergonz Mar 6, 2018
0e4fda8
fix: made the magic number smaller and a constant
xaviergonz Mar 6, 2018
c938269
fix: switched back to promise creation context
xaviergonz Mar 8, 2018
70169d3
fix: using a faster way to know if we are in a node context
xaviergonz Mar 8, 2018
9f69beb
Merge pull request #1 from nodejs/master
xaviergonz Apr 11, 2018
96a53db
Merge branch 'master' into fix-async-hooks-crash
xaviergonz Apr 11, 2018
cb3bba1
fix: rebase to latest master and use node_context_data constants
xaviergonz Apr 11, 2018
51257ed
fix: added namespace for enum
xaviergonz Apr 11, 2018
361d425
fix: storing actual pointers on the context
xaviergonz Apr 19, 2018
704b418
Merge branch 'master' into fix-async-hooks-crash
xaviergonz Apr 19, 2018
bca7fd1
src: code review
xaviergonz May 14, 2018
0577c29
fix: code review changes
xaviergonz May 14, 2018
54bf6dc
fix: code review
xaviergonz May 14, 2018
3c4d991
fix: code review changes
xaviergonz May 14, 2018
4d20b72
fix: code review
xaviergonz May 14, 2018
d9214a7
Merge branch 'master' into fix-async-hooks-crash
xaviergonz Jun 26, 2018
23fa418
src: fix async hooks crashing when there is no node context
xaviergonz Mar 4, 2018
c2b9b32
src: optimization for check for env promise hook
xaviergonz Mar 4, 2018
4437f90
src: fixed linting errors for env.cc
xaviergonz Mar 4, 2018
436919e
fix: updated with the latest code review
xaviergonz Mar 6, 2018
6b095d7
fix: made the magic number smaller and a constant
xaviergonz Mar 6, 2018
85cf945
fix: switched back to promise creation context
xaviergonz Mar 8, 2018
76df613
fix: using a faster way to know if we are in a node context
xaviergonz Mar 8, 2018
077c7a5
fix: rebase to latest master and use node_context_data constants
xaviergonz Apr 11, 2018
92ec8c1
fix: added namespace for enum
xaviergonz Apr 11, 2018
39b5c1f
fix: storing actual pointers on the context
xaviergonz Apr 19, 2018
9c72574
src: code review
xaviergonz May 14, 2018
e91cdc0
fix: code review changes
xaviergonz May 14, 2018
6ef891e
fix: code review
xaviergonz May 14, 2018
5ac8e62
fix: code review changes
xaviergonz May 14, 2018
aaa53b1
fix: code review
xaviergonz May 14, 2018
3a54578
fix: rebasing
xaviergonz Jun 26, 2018
634a63e
fix: linting errors
xaviergonz Jun 26, 2018
10b280e
fix: use actual magic number as ptr rather than ptr to int variable
xaviergonz Jun 26, 2018
e394201
fix: reinterpret cast and linting
xaviergonz Jun 26, 2018
af69d52
fix: better reinterpret cast
xaviergonz Jun 26, 2018
927d984
fix: reverting using fixed address
xaviergonz Jun 26, 2018
290b627
fix: fix for failing unit tests
xaviergonz Jun 27, 2018
93d7670
Merge branch 'master' into fix-async-hooks-crash
xaviergonz Jul 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,13 @@ bool Environment::EmitNapiWarning() {
void Environment::EnvPromiseHook(v8::PromiseHookType type,
v8::Local<v8::Promise> promise,
v8::Local<v8::Value> parent) {
Environment* env = Environment::GetCurrent(promise->CreationContext());
auto context = promise->CreationContext();
auto dataIndex = node::Environment::kContextEmbedderDataIndex;
// If the context is undefined (not a node context) then skip.
if (context->GetEmbedderData(dataIndex)->IsUndefined()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis is this still undefined behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I’m reading up on the V8 code, and it seems like a real issue here is that this might actually perform an out-of-bounds read if the context has less than dataIndex fields…

I assume the check is left out more or less intentionally on the V8 side, so I guess the options here are:

  • Add a method to v8::Context that tells us how many embedder data fields there are
  • Let Node attach some kind of property (e.g. a private symbol) on a context-specific object, like the global object or the ExtrasBindingObject, then use that to tell whether an Environment has been assigned to a context or not … this might be a bit slower but wouldn’t require changes to V8

/cc @nodejs/v8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @hashseed many a years ago I ran into similar trouble with the debug context and had similar thoughts to @addaleax. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about HasEmbedderData(index)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xaviergonz Do you have any thoughts on this? In the end, it’s your PR, so you obviously have some say in what happens here.

In particular, wrt the second option, that should be doable without waiting for anything else to happen, and it would be nice if you could indicate whether you’d want to try it yourself or prefer for somebody else to take a stab.

(If you do: We’d be really, really glad if we can help in any way. Don’t be shy to ask questions, here or on IRC!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat certain that Isolate::GetCurrentContext should give you the same context, since we run the microtask in the context of the Promise. If Object::CreationContext and Isolate::GetCurrentContext returns different contexts, you got a problem anyways, since Environment::GetCurrent uses the latter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashseed How do we get the isolate without Isolate::GetCurrent() or promise->GetIsolate()? Both seem worse than what we have now…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promise->GetIsolate get's it by masking off the object address to get to the heap page, where the isolate address is stored. That's not too bad.
Isolate::GetCurrent reads it from TLS, which can be slow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promise->GetIsolate get's it by masking off the object address to get to the heap page, where the isolate address is stored. That's not too bad.

V8_DEPRECATE_SOON("Keep track of isolate correctly", Isolate* GetIsolate());

😢

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah about that...

return;
}
Environment* env = Environment::GetCurrent(context);
for (const PromiseHookCallback& hook : env->promise_hooks_) {
hook.cb_(type, promise, parent, hook.arg_);
}
Expand Down