You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The indexed and named property setters and definers should only allow setting "the hole" as a return value -- not any other type. We don't currently check this in any way, allowing embedders to footgun themselves pretty easily.
On the C++ side, PropertyCallbackInfo<void> only accepts a single kind of return value: an empty handle. This is different from calling SetUndefined(), which sets undefined rather than TheHole!
Our tests currently call set_undefined on the ReturnValue which is incorrect. We should either add a set_empty to ReturnValue, and potentially should consider adding a more strongly-typed return value interface.
Deleters and property callbacks are boolean and integers as well, but those are probably easier for embedders to get right.
// Setting the hole value has different meanings depending on the usage:// - for function template callbacks it means that the callback returns// the undefined value,// - for property getter callbacks is means that the callback returns// the undefined value (for property setter callbacks the value returned// is ignored),// - for interceptor callbacks it means that the request was not handled.
V8_INLINE voidSetTheHole();
After further research, it looks like the return value is ignored for these with the new APIs.
// TODO(ishell): just return v8::Intercepted.
Handle<Object> PropertyCallbackArguments::CallIndexedSetter(
Handle<InterceptorInfo> interceptor, uint32_t index, Handle<Object> value) {
DCHECK(!interceptor->is_named());
Isolate* isolate = this->isolate();
RCS_SCOPE(isolate, RuntimeCallCounterId::kIndexedSetterCallback);
if (interceptor->has_new_callbacks_signature()) {
// New Api relies on the return value to be set to undefined.
// TODO(ishell): do this in the constructor once the old Api is deprecated.
slot_at(kReturnValueIndex).store(ReadOnlyRoots(isolate).undefined_value());
IndexedPropertySetterCallbackV2 f =
ToCData<IndexedPropertySetterCallbackV2>(interceptor->setter());
Handle<InterceptorInfo> has_side_effects;
PREPARE_CALLBACK_INFO_INTERCEPTOR(isolate, f, void, has_side_effects);
auto intercepted = f(index, v8::Utils::ToLocal(value), callback_info);
if (intercepted == v8::Intercepted::kNo) return {};
// Non-empty handle indicates that the request was intercepted.
return isolate->factory()->undefined_value();
} else {
IndexedPropertySetterCallback f =
ToCData<IndexedPropertySetterCallback>(interceptor->setter());
Handle<InterceptorInfo> has_side_effects;
PREPARE_CALLBACK_INFO_INTERCEPTOR(isolate, f, v8::Value, has_side_effects);
f(index, v8::Utils::ToLocal(value), callback_info);
return GetReturnValue<Object>(isolate);
}
}
The indexed and named property setters and definers should only allow setting "the hole" as a return value -- not any other type. We don't currently check this in any way, allowing embedders to footgun themselves pretty easily.
On the C++ side,
PropertyCallbackInfo<void>
only accepts a single kind of return value: an empty handle. This is different from callingSetUndefined()
, which setsundefined
rather thanTheHole
!Our tests currently call
set_undefined
on theReturnValue
which is incorrect. We should either add aset_empty
to ReturnValue, and potentially should consider adding a more strongly-typed return value interface.Deleters and property callbacks are boolean and integers as well, but those are probably easier for embedders to get right.
The text was updated successfully, but these errors were encountered: