Setting __proto__ on vm context's globalThis causes a crash (regression in v19.9.0)

[domenic]

Version

v20.0.0

Platform

Microsoft Windows NT 10.0.22621.0 x64

Subsystem

vm

What steps will reproduce the bug?

const vm = require("vm");
const context = vm.createContext();

const contextGlobalThis = vm.runInContext("this", context);

console.log("before");
contextGlobalThis.__proto__ = null;
console.log("after?");

This will only log "before" in Node v19.9.0 and v20.0.0. It will log both "before" and "after?" in Node v19.8.1.

If you log the exit code after a failed run I get 5.

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

Every time.

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

Setting the proto to null should work.

What do you see instead?

A crash

Additional information

This causes jsdom's test suite to crash as this sort of behavior is part of web platform tests.

[targos]

Debug trace:

    frame #3: 0x000000010022f7d8 node`node::Abort() at node_errors.cc:334:3
    frame #4: 0x000000010022f918 node`node::OnFatalError(location="v8::Object::Cast", message="Value is not an Object") at node_errors.cc:526:3
    frame #5: 0x00000001005629c0 node`v8::Object::CheckCast(v8::Value*) [inlined] v8::Utils::ReportApiFailure(location=<unavailable>, message=<unavailable>) at api.cc:322:5 [opt]
    frame #6: 0x000000010056296c node`v8::Object::CheckCast(v8::Value*) [inlined] v8::Utils::ApiCheck(condition=<unavailable>, location=<unavailable>, message=<unavailable>) at api.h:193:21 [opt]
    frame #7: 0x0000000100562964 node`v8::Object::CheckCast(that=<unavailable>) at api.cc:4111:3 [opt]
    frame #8: 0x000000010006d81c node`v8::Object::Cast(value=0x0000000110008210) at v8-object.h:779:3
    frame #9: 0x000000010006d7e4 node`v8::Local<v8::Object> v8::Local<v8::Object>::Cast<v8::Value>(that=(val_ = 0x0000000110008210)) at v8-local-handle.h:263:21
    frame #10: 0x000000010006af00 node`v8::Local<v8::Object> v8::Local<v8::Value>::As<v8::Object>(this=0x000000016fdfd6e8) const at v8-local-handle.h:273:12
    frame #11: 0x000000010020efe0 node`node::contextify::ContextifyContext::PropertySetterCallback(property=(val_ = 0x000000016fdfdc40), value=(val_ = 0x000000016fdfdc60), args=0x000000016fdfd7e0) at node_contextify.cc:539:35
    frame #12: 0x0000000100a68e10 node`v8::internal::PropertyCallbackArguments::CallNamedSetter(this=0x000000016fdfd870, interceptor=<unavailable>, name=Handle<v8::internal::Name> @ x20, value=Handle<v8::internal::Object> @ x21) at api-arguments-inl.h:215:3 [opt]
    frame #13: 0x0000000100cdf1bc node`v8::internal::(anonymous namespace)::SetPropertyWithInterceptorInternal(it=0x000000016fdfda00, interceptor=Handle<v8::internal::InterceptorInfo> @ x21, should_throw=(has_value_ = false, value_ = kThrowOnError), value=Handle<v8::internal::Object> @ x20) at js-objects.cc:1357:20 [opt]
    frame #14: 0x0000000100dfd1f0 node`v8::internal::Object::SetPropertyInternal(it=0x000000016fdfda00, value=Handle<v8::internal::Object> @ x19, should_throw=(has_value_ = false, value_ = kThrowOnError), store_origin=kNamed, found=0x000000016fdfd9af) at objects.cc:2515:15 [opt]
    frame #15: 0x0000000100dfd048 node`v8::internal::Object::SetProperty(it=0x000000016fdfda00, value=Handle<v8::internal::Object> @ x21, store_origin=kNamed, should_throw=(has_value_ = false, value_ = kThrowOnError)) at objects.cc:2633:9 [opt]
    frame #16: 0x0000000100a58240 node`v8::internal::StoreIC::Store(this=0x000000016fdfdaf0, object=Handle<v8::internal::Object> @ x23, name=Handle<v8::internal::Name> @ x21, value=Handle<v8::internal::Object> @ x19, store_origin=kNamed) at ic.cc:0 [opt]
    frame #17: 0x0000000100a5ef60 node`v8::internal::Runtime_StoreIC_Miss(int, unsigned long*, v8::internal::Isolate*) at ic.cc:2817:3 [opt]
    frame #18: 0x0000000100a5eb70 node`v8::internal::Runtime_StoreIC_Miss(args_length=<unavailable>, args_object=0x000000016fdfdc60, isolate=0x0000000110008000) at ic.cc:2790:1 [opt]
    frame #19: 0x00000001014fe5c4 node`Builtins_CEntry_Return1_ArgvOnStack_NoBuiltinExit + 100
    frame #20: 0x00000001015daee0 node`Builtins_SetNamedPropertyHandler + 160
    frame #21: 0x0000000101463404 node`Builtins_InterpreterEntryTrampoline + 260

[targos]

@nodejs/vm

[debadree25]

Hey! I ran a git bisect seems like this problem arose in #46615, I confirmed reverting the commit fixes this regression, although it seems that PR solved quite a lot of bugs so probably can blanket revert it, I am trying to understand the code if anyone has any more context on it that would be great! cc @dubzzz

Thank You!

[dubzzz]

Indeed the PR is solving many regressions introduced with the bump of v8 done at the beginning of node 18. There were multiple attempts to fix it, but it seems we still miss some cases.

For the record, I have another PR attempting to fix some remaining issues: #47572. But it will probably not fix the one reported here. Maybe I can check if I see something that could solve the reported issue, we might have something missing in my last attempt of fix.

In the meantime, @domenic did you checked if you have the issue on the head of 18 (highly probable) and head of 20 (bump of v8 done with 20 fixed some issues).

[debadree25]

looking at the stack trace I think this caused maybe because we are calling GetPropertyDescriptor() on a null value maybe thats causing it?

node/src/node_contextify.cc

Lines 530 to 533 in 03db049

	if (is_declared_on_sandbox &&
	ctx->sandbox()
	->GetOwnPropertyDescriptor(context, property)
	.ToLocal(&desc)) {

[dubzzz]

I have not read the stack yet but the null could be a good culprit. I'd rather suspect

node/src/node_contextify.cc

Lines 539 to 540 in 03db049

	if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) ||
	desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false))

[debadree25]

You are correct, putting checked putting some printf's around 😄, I think this needs some additional check but dont know what that could be

[dubzzz]

We should probably add a check for IsNull before doing the As cast 🤔

Something like:

  if (is_declared_on_sandbox && 
       ctx->sandbox() 
           ->GetOwnPropertyDescriptor(context, property) 
           .ToLocal(&desc) &&
       !desc.IsNull()
  ) { 
     Environment* env = Environment::GetCurrent(context); 
     Local<Object> desc_obj = desc.As<Object>();

That said, that's strange to receive null from a get property descriptor 🤔

[debadree25]

Oh ok, I doesnt actually return null, the condition is executed with even with this null check this part probably needs some check

node/src/node_contextify.cc

Lines 539 to 540 in 03db049

	if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) ||
	desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false))