FATAL ERROR v8::FromJust Maybe value is Nothing

[vdata1]

Version

v22.5.1

Platform

Debian 5.10.103-1  x86_64

Subsystem

No response

What steps will reproduce the bug?

Hi,
I want to report a node bug, by running the following code snippet, node.js gives a FATAL error.

 Object.defineProperty(Object.prototype, '0', {   
     set() {
        console.log(123);        
     }
 });

const http = require('http');

AH

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

By just running the given code, node runtime gives node crash.

What is the expected behavior? Why is that the expected behavior?

Not a crash, by looking at the stack trace, it seems it is connected to v8::FromJust Maybe value is Nothing.

What do you see instead?

FATAL ERROR: v8::FromJust Maybe value is Nothing
----- Native stack trace -----

 1: 0xe22fb7 node::OnFatalError(char const*, char const*) [node]
 2: 0x122ada6 v8::Utils::ReportApiFailure(char const*, char const*) [node]
 3: 0x10da3d5 node::TTYWrap::GetWindowSize(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 4: 0x7f44475cf6e2 

----- JavaScript stack trace -----

1: WriteStream (node:tty:115:28)
2: createWritableStdioStream (node:internal/bootstrap/switches/is_main_thread:56:16)
3: getStdout (node:internal/bootstrap/switches/is_main_thread:153:12)
4: get (node:internal/console/constructor:207:42)
5: value (node:internal/console/constructor:338:50)
6: log (node:internal/console/constructor:385:61)
7: set (/home/abdullah/node-deno-bun/test.js:3:25)
8: WriteStream (node:tty:115:28)
9: createWritableStdioStream (node:internal/bootstrap/switches/is_main_thread:56:16)
10: getStdout (node:internal/bootstrap/switches/is_main_thread:153:12)


Aborted

Additional information

No response

[vdata1]

I also tested it on Mac OS, using the same node version, and I got the same FATAL ERROR.

[aduh95]

I'm able to reproduce on latest main as well as on 20.14.0.

/cc @nodejs/v8

[targos]

Any reason to believe this is a V8 bug?

It looks like FromJust is used somewhere in core with an empty Maybe (which is the equivalent of ignoring errors and crashes the process).

[targos]

This is due to node creating holey arrays in some places. Then the setter is called when the first element is set.

In this case, it happens here:

node/lib/tty.js

Lines 114 to 115 in 67f7137

	const winSize = new Array(2);
	const err = this._handle.getWindowSize(winSize);

That's easy to fix with an initialization like const winSize = [0, 0];, but then it crashes at another place, for the same reason:

node/src/node_http_parser.cc

Lines 1306 to 1325 in 67f7137

	Local<Array> methods = Array::New(env->isolate());
	Local<Array> all_methods = Array::New(env->isolate());
	size_t method_index = -1;
	size_t all_method_index = -1;
	#define V(num, name, string) \
	methods \
	->Set(env->context(), \
	++method_index, \
	FIXED_ONE_BYTE_STRING(env->isolate(), #string)) \
	.Check();
	HTTP_METHOD_MAP(V)
	#undef V
	#define V(num, name, string) \
	all_methods \
	->Set(env->context(), \
	++all_method_index, \
	FIXED_ONE_BYTE_STRING(env->isolate(), #string)) \
	.Check();
	HTTP_ALL_METHOD_MAP(V)
	#undef V

[targos]

I think using CreateDataProperty instead of Set would avoid the issue but there are countless places where we can do it, as it's not specific to array indices:

Object.defineProperty(Object.prototype, 'methods', {   
  set() {
    throw new Error('boom')    
  }
});

const http = require('http');

[joyeecheung]

We could use the alternative Array::New and Object::New variants that take arrays of keys and/or values and copies them directly to avoid it, but then this looks like another case of invalid modifications to prototypes that we don't explicitly support

[targos]

We don't support them, but we usually try to avoid hard crashes in these cases.

[joyeecheung]

I have a PR to get rid of the crashes #54276 but I am hesitant to write a regression tests for these, because monkey patching prototypes are generally considered unsupported, there are also many other bindings where we don't care about this...well, it's worth the code cleanup anyway.

[targos]

I don't think we should add tests.