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

xaviergonz · 2018-03-04T18:31:14Z

This commits basically skips any promise env hooks when there's no node context currently active, which would otherwise end up in a crash.

Fixes: #19104

Checklist

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

Affected core subsystem(s)

src (async_hooks)

Fixes: nodejs#19104

addaleax

LGTM modulo style nit(s), thanks!

addaleax · 2018-03-04T18:37:56Z

src/env.cc

-  Environment* env = Environment::GetCurrent(promise->CreationContext());
+  auto context = promise->CreationContext();
+  // if the context is undefined (not a node context) then skip
+  if (context->GetEmbedderData(node::Environment::kContextEmbedderDataIndex)->IsUndefined()) {


I think make lint should give you an error, because this line is longer than 80 characters?

Also, maybe look at the C++ style guide, e.g. comments should ideally be capitalized and punctuated. :)

both done :)

devsnek · 2018-03-04T18:46:37Z

src/env.cc

+  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()) {


@bnoordhuis is this still undefined behavior?

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

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

how about HasEmbedderData(index)

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

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.

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

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.

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());

😢

Yeah about that...

xaviergonz · 2018-03-06T20:33:03Z

Just updated with all the comments

xaviergonz · 2018-03-06T20:35:12Z

Btw, the magic number is the hex number for ascii "node" :)

hashseed · 2018-03-07T09:11:40Z

So, I discussed this with @bmeurer a bit.

Scratch my comment about not using CreationContext. In this case you actually need to use it since the Isolate::GetCurrentContext gives you the context in which you resolved the promise, which may differ from the context where the promise was created.
This approach probably going to kill promise hook performance.
How about we get v8::Context::GetNumberOfDataSlots landed upstream and then float the patch on node, then use it to fix this issue?

xaviergonz · 2018-03-07T17:39:32Z

Still, even if that api is available, wouldn't we need two embedded data indexes to make sure that it is a node context?

Something like:
if

we have >= 32 embedded data indexes
and number 31 is set to a magic number
then we are pretty sure 32 is a pointer to a node env class instance

vs
if

we have >= 32 embedded data indexes
then we are sure 32 is a pointer to a node env class instance (are we? it could be anything else as long as the embedder used it for something else)

xaviergonz · 2018-03-07T17:42:01Z

Also I was wondering, (about the other approach that does not rely on v8 api changing) is it ok to set the embedder index with the magic number inside the promise template itself? that way you wouldn't need to access isolate/global.process at all I think, just:

if promise->getinternalfield(32 or whatever) is the magic number, we are on a node context

hashseed · 2018-03-07T20:10:29Z

Still, even if that api is available, wouldn't we need two embedded data indexes to make sure that it is a node context?

To be extra sure, with a clean solution, yes.

But we know that Chromium contexts only uses two slots. And assuming this does not change dramatically, we can just check the number of slots against 33 for Node.js.

xaviergonz · 2018-03-08T16:44:46Z

For now I have just switched back to promise->GetCreationContext()

@hashseed @addaleax what should I do with the pull request then? wait for v8 to change? :)

xaviergonz · 2018-03-08T16:53:13Z

Another thing I thought about is having a cache, a map<context address, boolean> that for each EnvPromiseHook does:

if (map does not have an entry for the context address then) {
  bool onNodeContext = process && process first internal field is magic number;
  map[promise creation context address] = onNodeContext;
}
if (!map[promise creation context address]) { return; }
else we are on a node context

pros: only need to make the expensive operation once per context, no v8 changes needed
cons: you waste 1 byte (maybe 4?) of memory per context creation

hashseed · 2018-03-08T17:02:00Z

Contexts live on V8's heap and can change their addresses. Also, where would the map live? As a static global value?

xaviergonz · 2018-03-08T17:04:04Z

Yes, a static global value, so you'd be leaking 1-4 byte per context that creates a promise.
Is there a way to uniquely identify a context then?

Though even when it changes the address the only thing that'd happen is that there would be a cache miss and it would need to check for the magic number again (I don't know how often a context changes its address).

hashseed · 2018-03-08T17:11:01Z

I'd just wait for the change in V8 and float it to use it in this fix.

I'm actually wondering whether this should be fixed in Electron instead. That's the only case where embedder data in contexts are used in two different ways.

xaviergonz · 2018-03-08T17:12:05Z

Would need to be changed in chromium then, since it is basically using the chromium generated context in those cases.

hashseed · 2018-03-08T17:15:52Z

Neither Chromium nor Node.js run into this issue on their own because embedder data are not designed to be used for two different embedders in the same isolate. I'd expect Electron to float a fix because it uses Node.js and Chromium contexts side by side.

xaviergonz · 2018-03-08T17:18:26Z

Is it possible for electron to change the embedder data of a chromium context?

xaviergonz · 2018-03-08T17:25:38Z

There would be a case where it would fail though now that I think about it, if two different contexts (one node and another chromium) eventually swapped places in memory and ended up being in the same address, but I don't know if that's possible.

xaviergonz · 2018-03-08T17:45:00Z

Ok, better idea :D

on env start:

  // Used by EnvPromiseHook to know that we are on a node context.
  isolate()->GetCurrentContext()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex + 1, (void *)kNodeContextTag);

then on env promise hook

void Environment::EnvPromiseHook(v8::PromiseHookType type,
                                 v8::Local<v8::Promise> promise,
                                 v8::Local<v8::Value> parent) {
  Local<v8::Context> context = promise->CreationContext();

  // Grow the embedder data if necessary to make sure we are not out of bounds
  // when reading the magic number.
  context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex + 2, nullptr);
  int magicNumber = (int)context->GetAlignedPointerFromEmbedderData(kContextEmbedderDataIndex + 1);
  if (magicNumber != kNodeContextTag) {
    return;
  }

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

pros: no changes to v8, fast
cons: storing a number instead of a pointer looks weird I guess? using embedder fields 32, 33 and 34 instead of just 32

xaviergonz · 2018-03-08T18:04:23Z

Updated to pull request to the latest idea. Confirmed to be working with electron.

xaviergonz · 2018-03-11T08:21:35Z

@addaleax any thoughts?

xaviergonz · 2018-06-26T20:31:44Z

didn't work, same unit test failing, so reverting

xaviergonz · 2018-06-27T18:57:07Z

Ok, I found the offending test:

{
  const d = domain.create();

  d.run(common.mustCall(() => {
    vm.runInNewContext(`
      const promise = Promise.resolve();
      assert.strictEqual(promise.domain, undefined);
      promise.then(common.mustCall(() => {
        assert.strictEqual(process.domain, d); // process.domain is actually undefined
      }));
    `, { common, assert, process, d });
  }));
}

apparently
assert.strictEqual(process.domain, d);

process.domain becomes undefined, since the context is actually not a node context (due to vm.runInNewContext)

now, what I don't know is if this is something correct (vm.runInNewContext creates a non node context) or it is wrong and because vm.runInNewContext should actually create an environment.

@addaleax @apapirovski Any thoughts?

xaviergonz · 2018-06-27T20:07:17Z

Found the issue, moving the setAlignedPointerInEmbedderData to AssignToContext fixed it :)

addaleax

Sorry for being so absent – thanks, this definitely still looks good!

Let’s see that we can get this landed, and thanks for your patience.

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

addaleax · 2018-07-13T15:03:17Z

Looks like the number of commits in here is making trouble for the CI …

CI without rebase: https://ci.nodejs.org/job/node-test-pull-request/15851/

addaleax · 2018-07-15T23:43:40Z

Thanks for all the hard work!

Landed in fb87d8a 🎉

PR-URL: #19134 Fixes: #19104 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

xaviergonz · 2018-07-17T11:44:28Z

just wondering, in which version of node will it land? a pull request for electron depends on this one, that's why I ask 😁

addaleax · 2018-07-17T11:52:56Z

@xaviergonz It’s on the v10.x-staging branch, so it will most likely be in the next v10.x release (that is, most likely this or next week).

Since you’re particularly interested in having this in Electron, /cc @codebytere – I think Electron might be able to just cherry-pick this commit?

codebytere · 2018-07-17T16:21:00Z

@xaviergonz yes, we should be able to cherry-pick this back into our next node upgrade

xaviergonz · 2018-07-18T21:49:08Z

@codebytere nice! the relevant pull request for electron is electron/electron#12109 (comment)

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

6b76017

Fixes: nodejs#19104

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

src: optimization for check for env promise hook

84a1822

addaleax approved these changes Mar 4, 2018

View reviewed changes

src: fixed linting errors for env.cc

5e9128f

devsnek reviewed Mar 4, 2018

View reviewed changes

xaviergonz mentioned this pull request Mar 4, 2018

feat: use async_hooks instead of monkey patching to call activateUvLoop electron/electron#12109

Closed

jasnell approved these changes Mar 5, 2018

View reviewed changes

fix: updated with the latest code review

9690b86

fix: made the magic number smaller and a constant

0e4fda8

fix: switched back to promise creation context

c938269

fix: using a faster way to know if we are in a node context

70169d3

xaviergonz added 4 commits June 26, 2018 19:22

fix: use actual magic number as ptr rather than ptr to int variable

10b280e

fix: reinterpret cast and linting

e394201

fix: better reinterpret cast

af69d52

fix: reverting using fixed address

927d984

fix: fix for failing unit tests

290b627

addaleax approved these changes Jul 13, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 13, 2018

Merge branch 'master' into fix-async-hooks-crash

93d7670

addaleax closed this Jul 15, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

This was referenced Jul 18, 2018

2018-07-18 Version 10.7.0 (Current) @targos beevelop/docker-nodejs#207

Closed

2018-07-18 Version 10.7.0 (Current) @targos beevelop/docker-nodejs#208

Closed

This was referenced Jul 18, 2018

2018-07-18 Version 10.7.0 (Current) @targos beevelop/docker-nodejs#209

Closed

2018-07-18 Version 10.7.0 (Current) @targos beevelop/docker-nodejs#210

Closed

2018-07-18 Version 10.7.0 (Current) @targos beevelop/docker-nodejs#211

Closed

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 19, 2018

addaleax mentioned this pull request Sep 12, 2018

node-report: meld into core #22712

Merged

4 tasks

codebytere mentioned this pull request Oct 15, 2018

src: expose Environment::kNodeContextTagPtr #23672

Closed

2 tasks

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

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

Conversation

xaviergonz commented Mar 4, 2018 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaviergonz commented Mar 6, 2018

xaviergonz commented Mar 6, 2018

hashseed commented Mar 7, 2018

xaviergonz commented Mar 7, 2018 • edited Loading

xaviergonz commented Mar 7, 2018 • edited Loading

hashseed commented Mar 7, 2018

xaviergonz commented Mar 8, 2018

xaviergonz commented Mar 8, 2018

hashseed commented Mar 8, 2018

xaviergonz commented Mar 8, 2018 • edited Loading

hashseed commented Mar 8, 2018

xaviergonz commented Mar 8, 2018 • edited Loading

hashseed commented Mar 8, 2018

xaviergonz commented Mar 8, 2018

xaviergonz commented Mar 8, 2018

xaviergonz commented Mar 8, 2018 • edited Loading

xaviergonz commented Mar 8, 2018

xaviergonz commented Mar 11, 2018

xaviergonz commented Jun 26, 2018

xaviergonz commented Jun 27, 2018 • edited Loading

xaviergonz commented Jun 27, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Jul 13, 2018

addaleax commented Jul 15, 2018 • edited Loading

xaviergonz commented Jul 17, 2018

addaleax commented Jul 17, 2018

codebytere commented Jul 17, 2018

xaviergonz commented Jul 18, 2018

xaviergonz commented Mar 4, 2018 •

edited

Loading

xaviergonz commented Mar 7, 2018 •

edited

Loading

xaviergonz commented Mar 7, 2018 •

edited

Loading

xaviergonz commented Mar 8, 2018 •

edited

Loading

xaviergonz commented Mar 8, 2018 •

edited

Loading

xaviergonz commented Mar 8, 2018 •

edited

Loading

xaviergonz commented Jun 27, 2018 •

edited

Loading

addaleax commented Jul 15, 2018 •

edited

Loading