From d611bd896babd1aff6f2f5d827fee07546d2f63c Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Mon, 23 Jun 2014 08:53:27 +0000 Subject: [PATCH] Simplify access checks performed by GetOwnProperty BUG= R=dcarney@chromium.org Review URL: https://codereview.chromium.org/339553002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21917 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 106 +++++++++++++--------------------------- test/cctest/test-api.cc | 77 ----------------------------- 2 files changed, 33 insertions(+), 150 deletions(-) diff --git a/src/runtime.cc b/src/runtime.cc index a6d609c08..c3a564a24 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -1914,83 +1914,64 @@ static bool CheckAccessException(Object* callback, template static bool CheckGenericAccess( Handle receiver, - Handle holder, + Handle end, Key key, v8::AccessType access_type, bool (Isolate::*mayAccess)(Handle, Key, v8::AccessType)) { Isolate* isolate = receiver->GetIsolate(); - for (Handle current = receiver; - true; - current = handle(JSObject::cast(current->GetPrototype()), isolate)) { + for (Handle current = receiver; + !current.is_identical_to(end); + current = Object::GetPrototype(isolate, current)) { if (current->IsAccessCheckNeeded() && - !(isolate->*mayAccess)(current, key, access_type)) { + !(isolate->*mayAccess)( + Handle::cast(current), key, access_type)) { return false; } - if (current.is_identical_to(holder)) break; } return true; } -enum AccessCheckResult { - ACCESS_FORBIDDEN, - ACCESS_ALLOWED, - ACCESS_ABSENT -}; - - -static AccessCheckResult CheckPropertyAccess(Handle obj, - Handle name, - v8::AccessType access_type) { +static void CheckPropertyAccess(Handle obj, + Handle name, + v8::AccessType access_type) { + Isolate* isolate = obj->GetIsolate(); uint32_t index; if (name->AsArrayIndex(&index)) { + Handle next(obj->GetPrototype(), isolate); // TODO(1095): we should traverse hidden prototype hierachy as well. - if (CheckGenericAccess( - obj, obj, index, access_type, &Isolate::MayIndexedAccess)) { - return ACCESS_ALLOWED; + if (!CheckGenericAccess( + obj, next, index, access_type, &Isolate::MayIndexedAccess)) { + obj->GetIsolate()->ReportFailedAccessCheck(obj, access_type); } - - obj->GetIsolate()->ReportFailedAccessCheck(obj, access_type); - return ACCESS_FORBIDDEN; + return; } - Isolate* isolate = obj->GetIsolate(); LookupResult lookup(isolate); obj->LookupOwn(name, &lookup, true); - if (!lookup.IsProperty()) return ACCESS_ABSENT; - Handle holder(lookup.holder(), isolate); + Handle next = lookup.IsProperty() + ? handle(lookup.holder()->GetPrototype(), isolate) + : Handle::cast(isolate->factory()->null_value()); if (CheckGenericAccess >( - obj, holder, name, access_type, &Isolate::MayNamedAccess)) { - return ACCESS_ALLOWED; + obj, next, name, access_type, &Isolate::MayNamedAccess)) { + return; } // Access check callback denied the access, but some properties // can have a special permissions which override callbacks descision // (currently see v8::AccessControl). // API callbacks can have per callback access exceptions. - switch (lookup.type()) { - case CALLBACKS: - if (CheckAccessException(lookup.GetCallbackObject(), access_type)) { - return ACCESS_ALLOWED; - } - break; - case INTERCEPTOR: - // If the object has an interceptor, try real named properties. - // Overwrite the result to fetch the correct property later. - holder->LookupRealNamedProperty(name, &lookup); - if (lookup.IsProperty() && lookup.IsPropertyCallbacks()) { - if (CheckAccessException(lookup.GetCallbackObject(), access_type)) { - return ACCESS_ALLOWED; - } - } - break; - default: - break; + if (lookup.IsFound() && lookup.type() == INTERCEPTOR) { + lookup.holder()->LookupOwnRealNamedProperty(name, &lookup); + } + + if (lookup.IsPropertyCallbacks() && + CheckAccessException(lookup.GetCallbackObject(), access_type)) { + return; } isolate->ReportFailedAccessCheck(obj, access_type); - return ACCESS_FORBIDDEN; } @@ -2012,16 +1993,9 @@ MUST_USE_RESULT static MaybeHandle GetOwnProperty(Isolate* isolate, Handle name) { Heap* heap = isolate->heap(); Factory* factory = isolate->factory(); - // Due to some WebKit tests, we want to make sure that we do not log - // more than one access failure here. - AccessCheckResult access_check_result = - CheckPropertyAccess(obj, name, v8::ACCESS_HAS); + + CheckPropertyAccess(obj, name, v8::ACCESS_HAS); RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); - switch (access_check_result) { - case ACCESS_FORBIDDEN: return factory->false_value(); - case ACCESS_ALLOWED: break; - case ACCESS_ABSENT: return factory->undefined_value(); - } PropertyAttributes attrs = JSReceiver::GetOwnPropertyAttributes(obj, name); if (attrs == ABSENT) { @@ -2032,7 +2006,7 @@ MUST_USE_RESULT static MaybeHandle GetOwnProperty(Isolate* isolate, Handle accessors; bool has_accessors = JSObject::GetOwnPropertyAccessorPair(obj, name).ToHandle(&accessors); - Handle elms = isolate->factory()->NewFixedArray(DESCRIPTOR_SIZE); + Handle elms = factory->NewFixedArray(DESCRIPTOR_SIZE); elms->set(ENUMERABLE_INDEX, heap->ToBoolean((attrs & DONT_ENUM) == 0)); elms->set(CONFIGURABLE_INDEX, heap->ToBoolean((attrs & DONT_DELETE) == 0)); elms->set(IS_ACCESSOR_INDEX, heap->ToBoolean(has_accessors)); @@ -2046,27 +2020,13 @@ MUST_USE_RESULT static MaybeHandle GetOwnProperty(Isolate* isolate, Object); elms->set(VALUE_INDEX, *value); } else { - // Access checks are performed for both accessors separately. - // When they fail, the respective field is not set in the descriptor. Handle getter(accessors->GetComponent(ACCESSOR_GETTER), isolate); Handle setter(accessors->GetComponent(ACCESSOR_SETTER), isolate); - - if (!getter->IsMap() && CheckPropertyAccess(obj, name, v8::ACCESS_GET)) { - ASSERT(!isolate->has_scheduled_exception()); - elms->set(GETTER_INDEX, *getter); - } else { - RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); - } - - if (!setter->IsMap() && CheckPropertyAccess(obj, name, v8::ACCESS_SET)) { - ASSERT(!isolate->has_scheduled_exception()); - elms->set(SETTER_INDEX, *setter); - } else { - RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); - } + elms->set(GETTER_INDEX, *getter); + elms->set(SETTER_INDEX, *setter); } - return isolate->factory()->NewJSArrayWithElements(elms); + return factory->NewJSArrayWithElements(elms); } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index cd38f9158..e01b6bab8 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -9184,17 +9184,6 @@ TEST(AccessControl) { ExpectUndefined( "Object.getOwnPropertyDescriptor(other, 'js_accessor_p')"); - // Enable ACCESS_HAS. - allowed_access_type[v8::ACCESS_HAS] = true; - ExpectUndefined("other.js_accessor_p"); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get"); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set"); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').value"); - allowed_access_type[v8::ACCESS_HAS] = false; - // Enable both ACCESS_HAS and ACCESS_GET. allowed_access_type[v8::ACCESS_HAS] = true; allowed_access_type[v8::ACCESS_GET] = true; @@ -9202,45 +9191,13 @@ TEST(AccessControl) { ExpectString("other.js_accessor_p", "getter"); ExpectObject( "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get", getter); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set"); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').value"); - - allowed_access_type[v8::ACCESS_GET] = false; - allowed_access_type[v8::ACCESS_HAS] = false; - - // Enable both ACCESS_HAS and ACCESS_SET. - allowed_access_type[v8::ACCESS_HAS] = true; - allowed_access_type[v8::ACCESS_SET] = true; - - ExpectUndefined("other.js_accessor_p"); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get"); ExpectObject( "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set", setter); ExpectUndefined( "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').value"); - allowed_access_type[v8::ACCESS_SET] = false; allowed_access_type[v8::ACCESS_HAS] = false; - - // Enable both ACCESS_HAS, ACCESS_GET and ACCESS_SET. - allowed_access_type[v8::ACCESS_HAS] = true; - allowed_access_type[v8::ACCESS_GET] = true; - allowed_access_type[v8::ACCESS_SET] = true; - - ExpectString("other.js_accessor_p", "getter"); - ExpectObject( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get", getter); - ExpectObject( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set", setter); - ExpectUndefined( - "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').value"); - - allowed_access_type[v8::ACCESS_SET] = false; allowed_access_type[v8::ACCESS_GET] = false; - allowed_access_type[v8::ACCESS_HAS] = false; // Access an element with JS accessor. CompileRun("other[42] = 2"); @@ -9248,51 +9205,17 @@ TEST(AccessControl) { ExpectUndefined("other[42]"); ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42')"); - // Enable ACCESS_HAS. - allowed_access_type[v8::ACCESS_HAS] = true; - ExpectUndefined("other[42]"); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').get"); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').set"); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value"); - allowed_access_type[v8::ACCESS_HAS] = false; - // Enable both ACCESS_HAS and ACCESS_GET. allowed_access_type[v8::ACCESS_HAS] = true; allowed_access_type[v8::ACCESS_GET] = true; ExpectString("other[42]", "el_getter"); ExpectObject("Object.getOwnPropertyDescriptor(other, '42').get", el_getter); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').set"); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value"); - - allowed_access_type[v8::ACCESS_GET] = false; - allowed_access_type[v8::ACCESS_HAS] = false; - - // Enable both ACCESS_HAS and ACCESS_SET. - allowed_access_type[v8::ACCESS_HAS] = true; - allowed_access_type[v8::ACCESS_SET] = true; - - ExpectUndefined("other[42]"); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').get"); ExpectObject("Object.getOwnPropertyDescriptor(other, '42').set", el_setter); ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value"); - allowed_access_type[v8::ACCESS_SET] = false; allowed_access_type[v8::ACCESS_HAS] = false; - - // Enable both ACCESS_HAS, ACCESS_GET and ACCESS_SET. - allowed_access_type[v8::ACCESS_HAS] = true; - allowed_access_type[v8::ACCESS_GET] = true; - allowed_access_type[v8::ACCESS_SET] = true; - - ExpectString("other[42]", "el_getter"); - ExpectObject("Object.getOwnPropertyDescriptor(other, '42').get", el_getter); - ExpectObject("Object.getOwnPropertyDescriptor(other, '42').set", el_setter); - ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value"); - - allowed_access_type[v8::ACCESS_SET] = false; allowed_access_type[v8::ACCESS_GET] = false; - allowed_access_type[v8::ACCESS_HAS] = false; v8::Handle value; -- 2.34.1