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

Fatal exception instead of Maximum call stack size exceeded #26829

Closed
BridgeAR opened this issue Mar 20, 2019 · 4 comments
Closed

Fatal exception instead of Maximum call stack size exceeded #26829

BridgeAR opened this issue Mar 20, 2019 · 4 comments

Comments

@BridgeAR
Copy link
Member

The following code causes an fatal exception for me (Node.js master):

'use strict';

async function test() {
  await test();
}

(async () => {
  try {
    await test();
  } catch (err) {
    console.log(err);
  }
})();
./node t.js
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: 0x55c2b5bff520 node::Abort() [./node]
 2: 0x55c2b5c00d5e node::OnFatalError(char const*, char const*) [./node]
 3: 0x55c2b5df79aa v8::Utils::ReportApiFailure(char const*, char const*) [./node]
 4: 0x55c2b5cc4a39 node::TTYWrap::Initialize(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, void*) [./node]
 5: 0x55c2b5bde214 node::binding::GetInternalBinding(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
 6: 0x55c2b5e7a369  [./node]
 7: 0x55c2b5e7b2f6  [./node]
 8: 0x55c2b5e7ba26 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node]
 9: 0x55c2b6c6b80b  [./node]
Aborted (core dumped)

I expected it to trigger RangeError: Maximum call stack size exceeded.

@Hakerh400
Copy link
Contributor

Hakerh400 commented Mar 21, 2019

Probably similar to #21591 and v8#8053. Accessing console.log (or process.stdout.write) near recursion depth limit disables further stdio operations.

More general:

const f = () => {
  try{
    f();
  }catch{
    console.log('A');
  }
};
f();
setTimeout(() => console.log('B'), 250);
setTimeout(() => process.stdout.write('C'), 500);
setTimeout(() => require('fs').writeSync(1, 'D'), 1000);

Expected: ABCD
Actual: D (or sometimes AB, then crashes)
Ref

For this particular issue with async functions, I made a fix in cf2140e (if it looks good I can open PR), but it doesn't fix other issues mentioned here.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 21, 2019

It's not really about stdio, though, it would fail when doing ToLocalChecked on the maybe returned by FunctionTemplate::GetFunction when the stack limit is near, and the initializer of tty_wrap happens to do that. A simpler repro (it's easier to reproduce this with async recursions, because it's easier to schedule execution of some binding in the middle of the async recursion):

'use strict';

Promise.resolve().then(() => {  // Or process.nextTick()
  process.binding('tty_wrap');
  // or cares_wrap, fs_event_wrap, http_parser, tls_wrap...
});

async function test() {
  await test();
}

(async () => {
  await test();
})();

We probably need to vet the ToLocalChecked() usages across the code base (and try to return early when the maybe is empty) to eliminate this, the initializer of tty_wrap is one of the bindings that are the easiest to trigger though since it's loaded in process.stdout/stdin/stderr 's getters.

@Hakerh400
Copy link
Contributor

That seems correct. The linked issues may then be unrelated, as it reproduces with other wrappers too.

Fix in #26832

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2019

Fixes by 6fb32ac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants