Commit 8437ed16 authored by Igor Sheludko's avatar Igor Sheludko Committed by V8 LUCI CQ

[runtime] Add interceptors side effects detector

This CL introduces SideEffectDetectorScope which requires explicit
allowlisting of cases when side effects are allowed after calling
interceptor callbacks.
Side effects are not allowed when the callback does not intercept
the request.
The side effects detector is not enabled yet, it will be enabled in
a follow-up CL.

Bug: chromium:1310062
Change-Id: I805764920ed016cb37390aef7bb02cbdf5f72846
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3641172Reviewed-by: 's avatarToon Verwaest <verwaest@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80484}
parent ef77fe0f
......@@ -148,6 +148,17 @@ Handle<Object> FunctionCallbackArguments::Call(CallHandlerInfo handler) {
return GetReturnValue<Object>(isolate);
}
PropertyCallbackArguments::~PropertyCallbackArguments(){
#ifdef DEBUG
// TODO(chromium:1310062): enable this check.
// if (javascript_execution_counter_) {
// CHECK_WITH_MSG(javascript_execution_counter_ ==
// isolate()->javascript_execution_counter(),
// "Unexpected side effect detected");
// }
#endif // DEBUG
}
Handle<JSObject> PropertyCallbackArguments::CallNamedEnumerator(
Handle<InterceptorInfo> interceptor) {
DCHECK(interceptor->is_named());
......@@ -296,6 +307,10 @@ Handle<Object> PropertyCallbackArguments::CallAccessorGetter(
Handle<AccessorInfo> info, Handle<Name> name) {
Isolate* isolate = this->isolate();
RCS_SCOPE(isolate, RuntimeCallCounterId::kAccessorGetterCallback);
// Unlike interceptor callbacks we know that the property exists, so
// the callback is allowed to have side effects.
AcceptSideEffects();
AccessorNameGetterCallback f =
ToCData<AccessorNameGetterCallback>(info->getter());
return BasicCallNamedGetterCallback(f, name, info,
......@@ -307,6 +322,10 @@ Handle<Object> PropertyCallbackArguments::CallAccessorSetter(
Handle<Object> value) {
Isolate* isolate = this->isolate();
RCS_SCOPE(isolate, RuntimeCallCounterId::kAccessorSetterCallback);
// Unlike interceptor callbacks we know that the property exists, so
// the callback is allowed to have side effects.
AcceptSideEffects();
AccessorNameSetterCallback f =
ToCData<AccessorNameSetterCallback>(accessor_info->setter());
PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, void, accessor_info,
......
......@@ -12,7 +12,12 @@ namespace internal {
PropertyCallbackArguments::PropertyCallbackArguments(
Isolate* isolate, Object data, Object self, JSObject holder,
Maybe<ShouldThrow> should_throw)
: Super(isolate) {
: Super(isolate)
#ifdef DEBUG
,
javascript_execution_counter_(isolate->javascript_execution_counter())
#endif // DEBUG
{
slot_at(T::kThisIndex).store(self);
slot_at(T::kHolderIndex).store(holder);
slot_at(T::kDataIndex).store(data);
......
......@@ -58,7 +58,15 @@ class CustomArguments : public CustomArgumentsBase {
// Note: Calling args.Call() sets the return value on args. For multiple
// Call()'s, a new args should be used every time.
class PropertyCallbackArguments
// This class also serves as a side effects detection scope (JavaScript code
// execution). It is used for ensuring correctness of the interceptor callback
// implementations. The idea is that the interceptor callback that does not
// intercept an operation must not produce side effects. If the callback
// signals that it has handled the operation (by either returning a respective
// result or by throwing an exception) then the AcceptSideEffects() method
// must be called to "accept" the side effects that have happened during the
// lifetime of the PropertyCallbackArguments object.
class PropertyCallbackArguments final
: public CustomArguments<PropertyCallbackInfo<Value> > {
public:
using T = PropertyCallbackInfo<Value>;
......@@ -74,6 +82,7 @@ class PropertyCallbackArguments
PropertyCallbackArguments(Isolate* isolate, Object data, Object self,
JSObject holder, Maybe<ShouldThrow> should_throw);
inline ~PropertyCallbackArguments();
// Don't copy PropertyCallbackArguments, because they would both have the
// same prev_ pointer.
......@@ -128,6 +137,14 @@ class PropertyCallbackArguments
inline Handle<JSObject> CallIndexedEnumerator(
Handle<InterceptorInfo> interceptor);
// Accept potential JavaScript side effects that might occurr during life
// time of this object.
inline void AcceptSideEffects() {
#ifdef DEBUG
javascript_execution_counter_ = 0;
#endif // DEBUG
}
private:
/*
* The following Call functions wrap the calling of all callbacks to handle
......@@ -148,6 +165,13 @@ class PropertyCallbackArguments
inline JSObject holder();
inline Object receiver();
#ifdef DEBUG
// This stores current value of Isolate::javascript_execution_counter().
// It's used for detecting whether JavaScript code was executed between
// PropertyCallbackArguments's constructior and destructor.
uint32_t javascript_execution_counter_;
#endif // DEBUG
};
class FunctionCallbackArguments
......
......@@ -377,6 +377,7 @@ V8_WARN_UNUSED_RESULT MaybeHandle<Object> Invoke(Isolate* isolate,
V8::GetCurrentPlatform()->DumpWithoutCrashing();
return isolate->factory()->undefined_value();
}
isolate->IncrementJavascriptExecutionCounter();
if (params.execution_target == Execution::Target::kCallable) {
Handle<Context> context = isolate->native_context();
......
......@@ -180,6 +180,16 @@ class StackMemory;
} \
} while (false)
#define RETURN_FAILURE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, detector) \
do { \
Isolate* __isolate__ = (isolate); \
DCHECK(!__isolate__->has_pending_exception()); \
if (__isolate__->has_scheduled_exception()) { \
detector.AcceptSideEffects(); \
return __isolate__->PromoteScheduledException(); \
} \
} while (false)
// Macros for MaybeHandle.
#define RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, value) \
......@@ -192,6 +202,10 @@ class StackMemory;
} \
} while (false)
#define RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, detector, value) \
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, \
(detector.AcceptSideEffects(), value))
#define RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, T) \
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, MaybeHandle<T>())
......@@ -518,6 +532,7 @@ using DebugObjectCache = std::vector<Handle<HeapObject>>;
V(bool, javascript_execution_assert, true) \
V(bool, javascript_execution_throws, true) \
V(bool, javascript_execution_dump, true) \
V(uint32_t, javascript_execution_counter, 0) \
V(bool, deoptimization_assert, true) \
V(bool, compilation_assert, true) \
V(bool, no_exception_assert, true)
......@@ -1655,6 +1670,10 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
return reinterpret_cast<Address>(&javascript_execution_assert_);
}
void IncrementJavascriptExecutionCounter() {
javascript_execution_counter_++;
}
Address handle_scope_implementer_address() {
return reinterpret_cast<Address>(&handle_scope_implementer_);
}
......
......@@ -3332,14 +3332,22 @@ RUNTIME_FUNCTION(Runtime_LoadPropertyWithInterceptor) {
isolate, receiver, Object::ConvertReceiver(isolate, receiver));
}
Handle<InterceptorInfo> interceptor(holder->GetNamedInterceptor(), isolate);
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*holder, Just(kDontThrow));
Handle<Object> result = arguments.CallNamedGetter(interceptor, name);
{
Handle<InterceptorInfo> interceptor(holder->GetNamedInterceptor(), isolate);
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*holder, Just(kDontThrow));
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
Handle<Object> result = arguments.CallNamedGetter(interceptor, name);
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, arguments);
if (!result.is_null()) return *result;
if (!result.is_null()) {
arguments.AcceptSideEffects();
return *result;
}
// If the interceptor didn't handle the request, then there must be no
// side effects.
}
LookupIterator it(isolate, receiver, name, holder);
// Skip any lookup work until we hit the (possibly non-masking) interceptor.
......@@ -3350,6 +3358,7 @@ RUNTIME_FUNCTION(Runtime_LoadPropertyWithInterceptor) {
}
// Skip past the interceptor.
it.Next();
Handle<Object> result;
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result, Object::GetProperty(&it));
if (it.IsFound()) return *result;
......@@ -3387,16 +3396,20 @@ RUNTIME_FUNCTION(Runtime_StorePropertyWithInterceptor) {
handle(JSObject::cast(receiver->map().prototype()), isolate);
}
DCHECK(interceptor_holder->HasNamedInterceptor());
Handle<InterceptorInfo> interceptor(interceptor_holder->GetNamedInterceptor(),
isolate);
{
Handle<InterceptorInfo> interceptor(
interceptor_holder->GetNamedInterceptor(), isolate);
DCHECK(!interceptor->non_masking());
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*receiver, Just(kDontThrow));
DCHECK(!interceptor->non_masking());
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*receiver, Just(kDontThrow));
Handle<Object> result = arguments.CallNamedSetter(interceptor, name, value);
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
if (!result.is_null()) return *value;
Handle<Object> result = arguments.CallNamedSetter(interceptor, name, value);
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, arguments);
if (!result.is_null()) return *value;
// If the interceptor didn't handle the request, then there must be no
// side effects.
}
LookupIterator it(isolate, receiver, name, receiver);
// Skip past any access check on the receiver.
......@@ -3426,7 +3439,7 @@ RUNTIME_FUNCTION(Runtime_LoadElementWithInterceptor) {
*receiver, Just(kDontThrow));
Handle<Object> result = arguments.CallIndexedGetter(interceptor, index);
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
RETURN_FAILURE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, arguments);
if (result.is_null()) {
LookupIterator it(isolate, receiver, index, receiver);
......@@ -3465,24 +3478,30 @@ RUNTIME_FUNCTION(Runtime_HasElementWithInterceptor) {
DCHECK_GE(args.smi_value_at(1), 0);
uint32_t index = args.smi_value_at(1);
Handle<InterceptorInfo> interceptor(receiver->GetIndexedInterceptor(),
isolate);
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*receiver, Just(kDontThrow));
if (!interceptor->query().IsUndefined(isolate)) {
Handle<Object> result = arguments.CallIndexedQuery(interceptor, index);
if (!result.is_null()) {
int32_t value;
CHECK(result->ToInt32(&value));
return value == ABSENT ? ReadOnlyRoots(isolate).false_value()
: ReadOnlyRoots(isolate).true_value();
}
} else if (!interceptor->getter().IsUndefined(isolate)) {
Handle<Object> result = arguments.CallIndexedGetter(interceptor, index);
if (!result.is_null()) {
return ReadOnlyRoots(isolate).true_value();
{
Handle<InterceptorInfo> interceptor(receiver->GetIndexedInterceptor(),
isolate);
PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver,
*receiver, Just(kDontThrow));
if (!interceptor->query().IsUndefined(isolate)) {
Handle<Object> result = arguments.CallIndexedQuery(interceptor, index);
if (!result.is_null()) {
int32_t value;
CHECK(result->ToInt32(&value));
if (value == ABSENT) return ReadOnlyRoots(isolate).false_value();
arguments.AcceptSideEffects();
return ReadOnlyRoots(isolate).true_value();
}
} else if (!interceptor->getter().IsUndefined(isolate)) {
Handle<Object> result = arguments.CallIndexedGetter(interceptor, index);
if (!result.is_null()) {
arguments.AcceptSideEffects();
return ReadOnlyRoots(isolate).true_value();
}
}
// If the interceptor didn't handle the request, then there must be no
// side effects.
}
LookupIterator it(isolate, receiver, index, receiver);
......
......@@ -1167,9 +1167,11 @@ MaybeHandle<Object> GetPropertyWithInterceptorInternal(
result = args.CallNamedGetter(interceptor, it->name());
}
RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, args,
MaybeHandle<Object>());
if (result.is_null()) return isolate->factory()->undefined_value();
*done = true;
args.AcceptSideEffects();
// Rebox handle before return
return handle(*result, isolate);
}
......@@ -1205,6 +1207,10 @@ Maybe<PropertyAttributes> GetPropertyAttributesWithInterceptorInternal(
CHECK(result->ToInt32(&value));
DCHECK_IMPLIES((value & ~PropertyAttributes::ALL_ATTRIBUTES_MASK) != 0,
value == PropertyAttributes::ABSENT);
// In case of absent property side effects are not allowed.
if (value != PropertyAttributes::ABSENT) {
args.AcceptSideEffects();
}
return Just(static_cast<PropertyAttributes>(value));
}
} else if (!interceptor->getter().IsUndefined(isolate)) {
......@@ -1215,10 +1221,14 @@ Maybe<PropertyAttributes> GetPropertyAttributesWithInterceptorInternal(
} else {
result = args.CallNamedGetter(interceptor, it->name());
}
if (!result.is_null()) return Just(DONT_ENUM);
if (!result.is_null()) {
args.AcceptSideEffects();
return Just(DONT_ENUM);
}
}
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<PropertyAttributes>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, args,
Nothing<PropertyAttributes>());
return Just(ABSENT);
}
......@@ -1252,7 +1262,9 @@ Maybe<bool> SetPropertyWithInterceptorInternal(
result = !args.CallNamedSetter(interceptor, it->name(), value).is_null();
}
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(it->isolate(), Nothing<bool>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(it->isolate(), args,
Nothing<bool>());
if (result) args.AcceptSideEffects();
return Just(result);
}
......@@ -1274,8 +1286,6 @@ Maybe<bool> DefinePropertyWithInterceptorInternal(
Object::ConvertReceiver(isolate, receiver),
Nothing<bool>());
}
PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
*holder, should_throw);
std::unique_ptr<v8::PropertyDescriptor> descriptor(
new v8::PropertyDescriptor());
......@@ -1298,6 +1308,8 @@ Maybe<bool> DefinePropertyWithInterceptorInternal(
descriptor->set_configurable(desc->configurable());
}
PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
*holder, should_throw);
if (it->IsElement(*holder)) {
result =
!args.CallIndexedDefiner(interceptor, it->array_index(), *descriptor)
......@@ -1307,7 +1319,9 @@ Maybe<bool> DefinePropertyWithInterceptorInternal(
!args.CallNamedDefiner(interceptor, it->name(), *descriptor).is_null();
}
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(it->isolate(), Nothing<bool>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(it->isolate(), args,
Nothing<bool>());
if (result) args.AcceptSideEffects();
return Just(result);
}
......@@ -1754,10 +1768,11 @@ Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
result = args.CallNamedDescriptor(interceptor, it->name());
}
// An exception was thrown in the interceptor. Propagate.
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, args, Nothing<bool>());
if (!result.is_null()) {
// Request successfully intercepted, try to set the property
// Request was successfully intercepted, try to set the property
// descriptor.
args.AcceptSideEffects();
Utils::ApiCheck(
PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc),
it->IsElement(*holder) ? "v8::IndexedPropertyDescriptorCallback"
......@@ -3918,10 +3933,11 @@ Maybe<bool> JSObject::DeletePropertyWithInterceptor(LookupIterator* it,
result = args.CallNamedDeleter(interceptor, it->name());
}
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate, args, Nothing<bool>());
if (result.is_null()) return Nothing<bool>();
DCHECK(result->IsBoolean());
args.AcceptSideEffects();
// Rebox CustomArguments::kReturnValueOffset before returning.
return Just(result->IsTrue(isolate));
}
......
......@@ -660,6 +660,7 @@ KeyAccumulator::FilterForEnumerableProperties(
if (!accessor->HasEntry(*result, entry)) continue;
// args are invalid after args.Call(), create a new one in every iteration.
// Query callbacks are not expected to have side effects.
PropertyCallbackArguments args(isolate_, interceptor->data(), *receiver,
*object, Just(kDontThrow));
......@@ -702,9 +703,14 @@ Maybe<bool> KeyAccumulator::CollectInterceptorKeysInternal(
result = enum_args.CallNamedEnumerator(interceptor);
}
}
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, Nothing<bool>());
RETURN_VALUE_IF_SCHEDULED_EXCEPTION_DETECTOR(isolate_, enum_args,
Nothing<bool>());
if (result.is_null()) return Just(true);
// Request was successfully intercepted, so accept potential side effects
// happened up to this point.
enum_args.AcceptSideEffects();
if ((filter_ & ONLY_ENUMERABLE) &&
!interceptor->query().IsUndefined(isolate_)) {
RETURN_NOTHING_IF_NOT_SUCCESSFUL(FilterForEnumerableProperties(
......
......@@ -40,6 +40,9 @@
# These tests are expected to hit a CHECK (i.e. a FAIL result actually means
# the test passed).
'test-verifiers/Fail*': [FAIL, CRASH],
# BUG(chromium:1310062): these tests must crash once the side effect check
# is enabled.
#'test-api-interceptors/Crash*': [FAIL, CRASH],
# This test always fails. It tests that LiveEdit causes abort when turned off.
'test-debug/LiveEditDisabled': [FAIL],
......
......@@ -701,7 +701,7 @@ THREADED_TEST(GlobalObjectAccessor) {
static void EmptyGetter(Local<Name> name,
const v8::PropertyCallbackInfo<v8::Value>& info) {
ApiTestFuzzer::Fuzz();
// The request is not intercepted so don't call ApiTestFuzzer::Fuzz() here.
}
......
This diff is collapsed.
......@@ -7998,7 +7998,8 @@ static void PGetter2(Local<Name> name,
const v8::PropertyCallbackInfo<v8::Value>& info) {
ApiTestFuzzer::Fuzz();
p_getter_count2++;
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
v8::Isolate* isolate = info.GetIsolate();
v8::Local<v8::Context> context = isolate->GetCurrentContext();
v8::Local<v8::Object> global = context->Global();
CHECK(
info.Holder()
......@@ -8025,6 +8026,8 @@ static void PGetter2(Local<Name> name,
global->Get(context, v8_str("o4")).ToLocalChecked())
.FromJust());
}
// Return something to indicate that the operation was intercepted.
info.GetReturnValue().Set(True(isolate));
}
......@@ -10226,7 +10229,7 @@ THREADED_TEST(InstanceProperties) {
static void GlobalObjectInstancePropertiesGet(
Local<Name> key, const v8::PropertyCallbackInfo<v8::Value>&) {
ApiTestFuzzer::Fuzz();
// The request is not intercepted so don't call ApiTestFuzzer::Fuzz() here.
}
static int script_execution_count = 0;
......@@ -11584,7 +11587,7 @@ THREADED_TEST(HandleIteration) {
static void InterceptorCallICFastApi(
Local<Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
ApiTestFuzzer::Fuzz();
// The request is not intercepted so don't call ApiTestFuzzer::Fuzz() here.
CheckReturnValue(info, FUNCTION_ADDR(InterceptorCallICFastApi));
int* call_count =
reinterpret_cast<int*>(v8::External::Cast(*info.Data())->Value());
......@@ -12156,6 +12159,8 @@ static void ShouldThrowOnErrorSetter(Local<Name> name, Local<v8::Value> value,
->Set(isolate->GetCurrentContext(), v8_str("should_throw_setter"),
should_throw_on_error_value)
.FromJust());
// Return a boolean to indicate that the operation was intercepted.
info.GetReturnValue().Set(True(isolate));
}
......@@ -12224,6 +12229,8 @@ static void ShouldThrowOnErrorDeleter(
->Set(isolate->GetCurrentContext(), v8_str("should_throw_deleter"),
should_throw_on_error_value)
.FromJust());
// Return a boolean to indicate that the operation was intercepted.
info.GetReturnValue().Set(True(isolate));
}
......@@ -12986,6 +12993,11 @@ int ApiTestFuzzer::current_;
// We are in a callback and want to switch to another thread (if we
// are currently running the thread fuzzing test).
void ApiTestFuzzer::Fuzz() {
// Emulate context switch which might cause side effects as well.
// This is mostly to ensure that the callbacks in the tests do not cause
// side effects when they don't intercept the operation.
CcTest::i_isolate()->IncrementJavascriptExecutionCounter();
if (!fuzzing_) return;
ApiTestFuzzer* test = RegisterThreadedTest::nth(current_)->fuzzer_;
test->ContextSwitch();
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment