async_hooks Segmentation fault

[szmarczak]

• Version: v13.13.0, v14.0.0
• Platform: Linux solus 5.5.11-151.current #1 SMP PREEMPT Tue Mar 24 18:06:46 UTC 2020 x86_64 GNU/Linux
• Subsystem: async_hooks, http

What steps will reproduce the bug?

$ node --stack-size=10240 bug.js

const got = require('got');
const async_hooks = require('async_hooks');

const asyncHook = async_hooks.createHook({
	init: (asyncId, type, triggerAsyncId, resource) => {
		console.log(`init ${asyncId} ${type} ${triggerAsyncId} ${resource}`);
	},
	before: asyncId => {
		console.log(`before ${asyncId}`);
	},
	after: asyncId => {
		console.log(`after ${asyncId}`);
	},
	destroy: asyncId => {
		console.log(`destroy ${asyncId}`);
	},
	promiseResolve: asyncId => {
		console.log(`promiseResolve ${asyncId}`);
	}
});

asyncHook.enable();

got('https://www.google.com').then(() => {
	asyncHook.disable();
});

How often does it reproduce? Is there a required condition?

On my machine - always. I've installed node via nvm. Tried nve too. The same result.

What is the expected behavior?

It should print logs. There's an infinite loop. It should throw Maximum call stack size exceeded.

What do you see instead?

Segmentation fault

[himself65]

replace console.log('xxx') to fs.writeSync(1, 'xxx') maybe works

[szmarczak]

Thanks for the idea. I buffered the logs and printed them after asyncHook.disable(). Worked like a charm! But should it print Segmentation fault?

[szmarczak]

Ah, there's an infinite loop. 🤦‍♂️

[himself65]

But should it print Segmentation fault?

maybe there have a bug, or just overflow the memories. whatever, I'll check it later

[Hakerh400]

It should not segfault. Infinite recursion should only throw a RangeError, not terminate the process. Node.js is probably miscalculating stack size. I cannot reproduce segfault on Windows (I see RangeError thrown and exit code 1)

[himself65]

It should not segfault. Infinite recursion should only throw a RangeError, not terminate the process. Node.js is probably miscalculating stack size. I cannot reproduce segfault on Windows (I see RangeError thrown and exit code 1)

Yes. Basically, I expect the output error is RangeError: Maximum call stack size exceeded. But Segmentation fault would be an unexpected error for me

[szmarczak]

Async Hook -> console.log -> Async Hook -> console.log -> ... you get the idea

[himself65]

Could we add a feature that will throw an error when the user calls a loop async_hooks? I think it more useful for coding experience instead of CPU full running and stack exceeded.

/cc @nodejs/async_hooks

[jiripospisil]

I can reproduce it on Linux with the latest official version:

(gdb) run
Starting program: /home/bob/Downloads/node-v14.0.0-linux-x64/bin/node --stack-size=10240 kk.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff6c81700 (LWP 7203)]
[New Thread 0x7ffff6480700 (LWP 7204)]
[New Thread 0x7ffff5c7f700 (LWP 7205)]
[New Thread 0x7ffff547e700 (LWP 7206)]
[New Thread 0x7ffff4c7d700 (LWP 7207)]
[New Thread 0x7ffff7ff6700 (LWP 7208)]
[New Thread 0x7fffdffff700 (LWP 7209)]
[New Thread 0x7fffdf7fe700 (LWP 7210)]
[New Thread 0x7fffdeffd700 (LWP 7211)]
[New Thread 0x7fffde7fc700 (LWP 7212)]

Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x0000000000b29cfc in node::inspector::(anonymous namespace)::InspectorConsoleCall(v8::FunctionCallbackInfo<v8::Value> const&) ()
(gdb) bt
#0  0x0000000000b29cfc in node::inspector::(anonymous namespace)::InspectorConsoleCall(v8::FunctionCallbackInfo<v8::Value> const&) ()
#1  0x0000000000c02b0b in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#2  0x0000000000c040b6 in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) ()
#3  0x0000000000c04736 in v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) ()
#4  0x00000000013a6339 in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit ()
#5  0x000000000133ee18 in Builtins_InterpreterEntryTrampoline ()
#6  0x000025ca88a40471 in ?? ()
#7  0x0000083826343cb9 in ?? ()
#8  0x0000000800000000 in ?? ()

Running without stack-size results in RangeError: Maximum call stack size exceeded as expected.

[himself65]

@jiripospisil great, now we know where the bug is

[addaleax]

Running without stack-size results in RangeError: Maximum call stack size exceeded as expected.

--stack-size tells V8 how much stack it is allowed to use before reporting a Maximum call stack size exceeded error, but does not specify the actual stack size that will be available. Passing a value that’s larger than the actual stack will lead to a segmentation fault due to stack overflow because it tells V8 to make wrong assumptions about the stack depth.

[szmarczak]

how much stack it is allowed

Exactly, my point is on the emphasis of allowed. Because it's allowed to allocate more than it should, it doesn't mean that it needs to. I suggest either changing the behavior or changing the --v8-options help page.

but does not specify the actual stack size that will be available.

Then what's the point of having this option?

[addaleax]

@szmarczak V8 or Node.js don’t allocate the main thread stack – that’s done by the OS. You can control that value through ulimit -s (but keep in mind that some extra space is needed for being able to handle the exception itself or for calls into C++ from JS).

I suggest either changing the behavior or changing the --v8-options help page.

I don’t think changing the behavior is an option here.

Changing the help page would not be an issue – what would be a good, succinct description for --stack-size here? size of stack region assumed to be available for V8?

but does not specify the actual stack size that will be available.

Then what's the point of having this option?

The point is that people who know that their OS provides more stack space than V8 assumes, and want deeper recursion depths, can inform V8 about that.

[szmarczak]

size of stack region assumed to be available for V8

Sounds good. What does default mean here:

default size of stack region v8 is allowed to use (in kBytes)

default usually implies that the value is dynamically changed. If it's not the case, I propose this:

the size of stack region v8 needs to allocate (in kBytes)

[addaleax]

@szmarczak I'm not sure whether default is included intentionally there, actually.

If it's not the case, I propose this:

the size of stack region v8 needs to allocate (in kBytes)

I think that wording would be misleading because it makes it sound like V8 allocates the stack region.

[szmarczak]

Do you know what file is responsible for the contents of help page for the v8 options?

[targos]

It's defined here: https://source.chromium.org/chromium/chromium/src/+/master:v8/src/flags/flag-definitions.h;l=1182-1184;drc=3f270f34a13929f895554778ee0016338a292108?originalUrl=https:%2F%2Fcs.chromium.org%2F

[szmarczak]

Let me guess. So editing this in the Node.js repo wouldn't pass?

[addaleax]

@szmarczak Yes, this would have to be changed in V8. I assume that that wouldn’t be an issue, though.