From 4aa69977c74e31bc04f4ec5f304931b704e53c80 Mon Sep 17 00:00:00 2001 From: Igor Sheludko Date: Mon, 22 Apr 2024 13:38:36 +0200 Subject: [PATCH] Migrate to a new V8 interceptors Api (#180) The new callback should return v8::Intercepted::kYes/kNo to indicate whether the operation was intercepted. This replaces the old approach where the callback had to leave the return value unset or set it to an empty handle to indicate that the the request wasn't intercepted. See https://crrev.com/c/5465509 and https://crrev.com/c/5465513. --- src/node_contextify.cc | 113 ++++++++++++++++++---------------- src/node_contextify.h | 37 +++++------ src/node_env_var.cc | 50 +++++++++------ src/node_external_reference.h | 21 ++++--- 4 files changed, 119 insertions(+), 102 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 1bc99765ba9f6e..0fc72bb1aaa03d 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -49,6 +49,7 @@ using v8::FunctionTemplate; using v8::HandleScope; using v8::IndexedPropertyHandlerConfiguration; using v8::Int32; +using v8::Intercepted; using v8::Isolate; using v8::Just; using v8::Local; @@ -459,13 +460,12 @@ bool ContextifyContext::IsStillInitializing(const ContextifyContext* ctx) { } // static -void ContextifyContext::PropertyGetterCallback( - Local property, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::PropertyGetterCallback( + Local property, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Local context = ctx->context(); Local sandbox = ctx->sandbox(); @@ -482,18 +482,20 @@ void ContextifyContext::PropertyGetterCallback( rv = ctx->global_proxy(); args.GetReturnValue().Set(rv); + return Intercepted::kYes; } + return Intercepted::kNo; } // static -void ContextifyContext::PropertySetterCallback( +Intercepted ContextifyContext::PropertySetterCallback( Local property, Local value, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Local context = ctx->context(); PropertyAttribute attributes = PropertyAttribute::None; @@ -511,8 +513,7 @@ void ContextifyContext::PropertySetterCallback( (static_cast(attributes) & static_cast(PropertyAttribute::ReadOnly)); - if (read_only) - return; + if (read_only) return Intercepted::kNo; // true for x = 5 // false for this.x = 5 @@ -532,10 +533,11 @@ void ContextifyContext::PropertySetterCallback( bool is_declared = is_declared_on_global_proxy || is_declared_on_sandbox; if (!is_declared && args.ShouldThrowOnError() && is_contextual_store && !is_function) - return; + return Intercepted::kNo; - if (!is_declared && property->IsSymbol()) return; - if (ctx->sandbox()->Set(context, property, value).IsNothing()) return; + if (!is_declared && property->IsSymbol()) return Intercepted::kNo; + if (ctx->sandbox()->Set(context, property, value).IsNothing()) + return Intercepted::kNo; Local desc; if (is_declared_on_sandbox && @@ -549,19 +551,21 @@ void ContextifyContext::PropertySetterCallback( // We have to specify the return value for any contextual or get/set // property if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) || - desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false)) + desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false)) { args.GetReturnValue().Set(value); + return Intercepted::kYes; + } } + return Intercepted::kNo; } // static -void ContextifyContext::PropertyDescriptorCallback( - Local property, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::PropertyDescriptorCallback( + Local property, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Local context = ctx->context(); @@ -571,19 +575,21 @@ void ContextifyContext::PropertyDescriptorCallback( Local desc; if (sandbox->GetOwnPropertyDescriptor(context, property).ToLocal(&desc)) { args.GetReturnValue().Set(desc); + return Intercepted::kYes; } } + return Intercepted::kNo; } // static -void ContextifyContext::PropertyDefinerCallback( +Intercepted ContextifyContext::PropertyDefinerCallback( Local property, const PropertyDescriptor& desc, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Local context = ctx->context(); Isolate* isolate = context->GetIsolate(); @@ -602,7 +608,7 @@ void ContextifyContext::PropertyDefinerCallback( // If the property is set on the global as neither writable nor // configurable, don't change it on the global or sandbox. if (is_declared && read_only && dont_delete) { - return; + return Intercepted::kNo; } Local sandbox = ctx->sandbox(); @@ -625,6 +631,9 @@ void ContextifyContext::PropertyDefinerCallback( desc.has_set() ? desc.set() : Undefined(isolate).As()); define_prop_on_sandbox(&desc_for_sandbox); + // TODO(https://github.com/nodejs/node/issues/52634): this should return + // kYes to behave according to the expected semantics. + return Intercepted::kNo; } else { Local value = desc.has_value() ? desc.value() : Undefined(isolate).As(); @@ -636,26 +645,29 @@ void ContextifyContext::PropertyDefinerCallback( PropertyDescriptor desc_for_sandbox(value); define_prop_on_sandbox(&desc_for_sandbox); } + // TODO(https://github.com/nodejs/node/issues/52634): this should return + // kYes to behave according to the expected semantics. + return Intercepted::kNo; } + return Intercepted::kNo; } // static -void ContextifyContext::PropertyDeleterCallback( - Local property, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::PropertyDeleterCallback( + Local property, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Maybe success = ctx->sandbox()->Delete(ctx->context(), property); - if (success.FromMaybe(false)) - return; + if (success.FromMaybe(false)) return Intercepted::kNo; // Delete failed on the sandbox, intercept and do not delete on // the global object. args.GetReturnValue().Set(false); + return Intercepted::kYes; } // static @@ -675,76 +687,71 @@ void ContextifyContext::PropertyEnumeratorCallback( } // static -void ContextifyContext::IndexedPropertyGetterCallback( - uint32_t index, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::IndexedPropertyGetterCallback( + uint32_t index, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; - ContextifyContext::PropertyGetterCallback( + return ContextifyContext::PropertyGetterCallback( Uint32ToName(ctx->context(), index), args); } - -void ContextifyContext::IndexedPropertySetterCallback( +Intercepted ContextifyContext::IndexedPropertySetterCallback( uint32_t index, Local value, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; - ContextifyContext::PropertySetterCallback( + return ContextifyContext::PropertySetterCallback( Uint32ToName(ctx->context(), index), value, args); } // static -void ContextifyContext::IndexedPropertyDescriptorCallback( - uint32_t index, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::IndexedPropertyDescriptorCallback( + uint32_t index, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; - ContextifyContext::PropertyDescriptorCallback( + return ContextifyContext::PropertyDescriptorCallback( Uint32ToName(ctx->context(), index), args); } - -void ContextifyContext::IndexedPropertyDefinerCallback( +Intercepted ContextifyContext::IndexedPropertyDefinerCallback( uint32_t index, const PropertyDescriptor& desc, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; - ContextifyContext::PropertyDefinerCallback( + return ContextifyContext::PropertyDefinerCallback( Uint32ToName(ctx->context(), index), desc, args); } // static -void ContextifyContext::IndexedPropertyDeleterCallback( - uint32_t index, - const PropertyCallbackInfo& args) { +Intercepted ContextifyContext::IndexedPropertyDeleterCallback( + uint32_t index, const PropertyCallbackInfo& args) { ContextifyContext* ctx = ContextifyContext::Get(args); // Still initializing - if (IsStillInitializing(ctx)) return; + if (IsStillInitializing(ctx)) return Intercepted::kNo; Maybe success = ctx->sandbox()->Delete(ctx->context(), index); - if (success.FromMaybe(false)) - return; + if (success.FromMaybe(false)) return Intercepted::kNo; // Delete failed on the sandbox, intercept and do not delete on // the global object. args.GetReturnValue().Set(false); + return Intercepted::kYes; } void ContextifyScript::CreatePerIsolateProperties( diff --git a/src/node_contextify.h b/src/node_contextify.h index 66a9f765fe9212..171252037b48fe 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -111,42 +111,39 @@ class ContextifyContext : public BaseObject { v8::Local code); static void WeakCallback( const v8::WeakCallbackInfo& data); - static void PropertyGetterCallback( + static v8::Intercepted PropertyGetterCallback( v8::Local property, const v8::PropertyCallbackInfo& args); - static void PropertySetterCallback( + static v8::Intercepted PropertySetterCallback( v8::Local property, v8::Local value, - const v8::PropertyCallbackInfo& args); - static void PropertyDescriptorCallback( + const v8::PropertyCallbackInfo& args); + static v8::Intercepted PropertyDescriptorCallback( v8::Local property, const v8::PropertyCallbackInfo& args); - static void PropertyDefinerCallback( + static v8::Intercepted PropertyDefinerCallback( v8::Local property, const v8::PropertyDescriptor& desc, - const v8::PropertyCallbackInfo& args); - static void PropertyDeleterCallback( + const v8::PropertyCallbackInfo& args); + static v8::Intercepted PropertyDeleterCallback( v8::Local property, const v8::PropertyCallbackInfo& args); static void PropertyEnumeratorCallback( const v8::PropertyCallbackInfo& args); - static void IndexedPropertyGetterCallback( - uint32_t index, - const v8::PropertyCallbackInfo& args); - static void IndexedPropertySetterCallback( + static v8::Intercepted IndexedPropertyGetterCallback( + uint32_t index, const v8::PropertyCallbackInfo& args); + static v8::Intercepted IndexedPropertySetterCallback( uint32_t index, v8::Local value, - const v8::PropertyCallbackInfo& args); - static void IndexedPropertyDescriptorCallback( - uint32_t index, - const v8::PropertyCallbackInfo& args); - static void IndexedPropertyDefinerCallback( + const v8::PropertyCallbackInfo& args); + static v8::Intercepted IndexedPropertyDescriptorCallback( + uint32_t index, const v8::PropertyCallbackInfo& args); + static v8::Intercepted IndexedPropertyDefinerCallback( uint32_t index, const v8::PropertyDescriptor& desc, - const v8::PropertyCallbackInfo& args); - static void IndexedPropertyDeleterCallback( - uint32_t index, - const v8::PropertyCallbackInfo& args); + const v8::PropertyCallbackInfo& args); + static v8::Intercepted IndexedPropertyDeleterCallback( + uint32_t index, const v8::PropertyCallbackInfo& args); v8::Global context_; std::unique_ptr microtask_queue_; diff --git a/src/node_env_var.cc b/src/node_env_var.cc index bce7ae07214ddf..31bdf49cb55bb0 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -16,6 +16,7 @@ using v8::DontEnum; using v8::FunctionTemplate; using v8::HandleScope; using v8::Integer; +using v8::Intercepted; using v8::Isolate; using v8::Just; using v8::Local; @@ -336,24 +337,27 @@ Maybe KVStore::AssignToObject(v8::Isolate* isolate, return Just(true); } -static void EnvGetter(Local property, - const PropertyCallbackInfo& info) { +static Intercepted EnvGetter(Local property, + const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsSymbol()) { - return info.GetReturnValue().SetUndefined(); + info.GetReturnValue().SetUndefined(); + return Intercepted::kYes; } CHECK(property->IsString()); MaybeLocal value_string = env->env_vars()->Get(env->isolate(), property.As()); if (!value_string.IsEmpty()) { info.GetReturnValue().Set(value_string.ToLocalChecked()); + return Intercepted::kYes; } + return Intercepted::kNo; } -static void EnvSetter(Local property, - Local value, - const PropertyCallbackInfo& info) { +static Intercepted EnvSetter(Local property, + Local value, + const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); // calling env->EmitProcessEnvWarning() sets a variable indicating that @@ -370,34 +374,37 @@ static void EnvSetter(Local property, "value to a string before setting process.env with it.", "DEP0104") .IsNothing()) - return; + return Intercepted::kNo; } Local key; Local value_string; if (!property->ToString(env->context()).ToLocal(&key) || !value->ToString(env->context()).ToLocal(&value_string)) { - return; + return Intercepted::kNo; } env->env_vars()->Set(env->isolate(), key, value_string); - - // Whether it worked or not, always return value. - info.GetReturnValue().Set(value); + return Intercepted::kYes; } -static void EnvQuery(Local property, - const PropertyCallbackInfo& info) { +static Intercepted EnvQuery(Local property, + const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { int32_t rc = env->env_vars()->Query(env->isolate(), property.As()); - if (rc != -1) info.GetReturnValue().Set(rc); + if (rc != -1) { + // Return attributes for the property. + info.GetReturnValue().Set(v8::None); + return Intercepted::kYes; + } } + return Intercepted::kNo; } -static void EnvDeleter(Local property, - const PropertyCallbackInfo& info) { +static Intercepted EnvDeleter(Local property, + const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { @@ -407,6 +414,7 @@ static void EnvDeleter(Local property, // process.env never has non-configurable properties, so always // return true like the tc39 delete operator. info.GetReturnValue().Set(true); + return Intercepted::kYes; } static void EnvEnumerator(const PropertyCallbackInfo& info) { @@ -417,9 +425,9 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { env->env_vars()->Enumerate(env->isolate())); } -static void EnvDefiner(Local property, - const PropertyDescriptor& desc, - const PropertyCallbackInfo& info) { +static Intercepted EnvDefiner(Local property, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); if (desc.has_value()) { if (!desc.has_writable() || @@ -430,6 +438,7 @@ static void EnvDefiner(Local property, "configurable, writable," " and enumerable " "data descriptor"); + return Intercepted::kYes; } else if (!desc.configurable() || !desc.enumerable() || !desc.writable()) { @@ -438,6 +447,7 @@ static void EnvDefiner(Local property, "configurable, writable," " and enumerable " "data descriptor"); + return Intercepted::kYes; } else { return EnvSetter(property, desc.value(), info); } @@ -447,12 +457,14 @@ static void EnvDefiner(Local property, "'process.env' does not accept an" " accessor(getter/setter)" " descriptor"); + return Intercepted::kYes; } else { THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env, "'process.env' only accepts a " "configurable, writable," " and enumerable " "data descriptor"); + return Intercepted::kYes; } } diff --git a/src/node_external_reference.h b/src/node_external_reference.h index f66bdf40c2a9f8..510e12c0afeb0f 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -59,16 +59,17 @@ class ExternalReferenceRegistry { V(v8::FunctionCallback) \ V(v8::AccessorNameGetterCallback) \ V(v8::AccessorNameSetterCallback) \ - V(v8::GenericNamedPropertyDefinerCallback) \ - V(v8::GenericNamedPropertyDeleterCallback) \ - V(v8::GenericNamedPropertyEnumeratorCallback) \ - V(v8::GenericNamedPropertyQueryCallback) \ - V(v8::GenericNamedPropertySetterCallback) \ - V(v8::IndexedPropertySetterCallback) \ - V(v8::IndexedPropertyDefinerCallback) \ - V(v8::IndexedPropertyDeleterCallback) \ - V(v8::IndexedPropertyQueryCallback) \ - V(v8::IndexedPropertyDescriptorCallback) \ + V(v8::NamedPropertyGetterCallback) \ + V(v8::NamedPropertyDefinerCallback) \ + V(v8::NamedPropertyDeleterCallback) \ + V(v8::NamedPropertyEnumeratorCallback) \ + V(v8::NamedPropertyQueryCallback) \ + V(v8::NamedPropertySetterCallback) \ + V(v8::IndexedPropertyGetterCallbackV2) \ + V(v8::IndexedPropertySetterCallbackV2) \ + V(v8::IndexedPropertyDefinerCallbackV2) \ + V(v8::IndexedPropertyDeleterCallbackV2) \ + V(v8::IndexedPropertyQueryCallbackV2) \ V(const v8::String::ExternalStringResourceBase*) #define V(ExternalReferenceType) \