From f0d555b26ddbf7f825e78928197932b45db2ac8a Mon Sep 17 00:00:00 2001 From: machenbach Date: Wed, 25 Mar 2015 11:31:36 -0700 Subject: [PATCH] Revert of add access checks to receivers on function callbacks (patchset #5 id:80001 of https://codereview.chromium.org/1036743004/) Reason for revert: This seems to lead to lots of timeouts of layout tests, e.g.: http://build.chromium.org/p/client.v8/builders/V8-Blink%20Linux%2064/builds/2807 Original issue's description: > add access checks to receivers on function callbacks > > R=verwaest@chromium.org > BUG=468451 > LOG=N > > Committed: https://crrev.com/255528710b0a128eef7b66827d9ac43e44650ff4 > Cr-Commit-Position: refs/heads/master@{#27452} TBR=verwaest@chromium.org,dcarney@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=468451 Review URL: https://codereview.chromium.org/1023783009 Cr-Commit-Position: refs/heads/master@{#27457} --- src/builtins.cc | 12 ++++--- src/hydrogen.cc | 8 ----- src/objects.cc | 19 ++++------ src/objects.h | 4 +-- test/cctest/test-accessors.cc | 83 ------------------------------------------- test/cctest/test-api.cc | 7 ++-- 6 files changed, 16 insertions(+), 117 deletions(-) diff --git a/src/builtins.cc b/src/builtins.cc index f4d9aa2..71867e1 100644 --- a/src/builtins.cc +++ b/src/builtins.cc @@ -1044,9 +1044,7 @@ MUST_USE_RESULT static MaybeHandle HandleApiCallHelper( DCHECK(!args[0]->IsNull()); if (args[0]->IsUndefined()) args[0] = function->global_proxy(); - Handle receiver(&args[0]); - Handle raw_holder = - fun_data->GetCompatibleReceiver(isolate, receiver, is_construct); + Object* raw_holder = fun_data->GetCompatibleReceiver(isolate, args[0]); if (raw_holder->IsNull()) { // This function cannot be called with the given receiver. Abort! @@ -1068,8 +1066,12 @@ MUST_USE_RESULT static MaybeHandle HandleApiCallHelper( LOG(isolate, ApiObjectAccess("call", JSObject::cast(*args.receiver()))); DCHECK(raw_holder->IsJSObject()); - FunctionCallbackArguments custom(isolate, data_obj, *function, *raw_holder, - &args[0] - 1, args.length() - 1, + FunctionCallbackArguments custom(isolate, + data_obj, + *function, + raw_holder, + &args[0] - 1, + args.length() - 1, is_construct); v8::Handle value = custom.Call(callback); diff --git a/src/hydrogen.cc b/src/hydrogen.cc index c4b446e..230939f 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -8646,14 +8646,6 @@ bool HOptimizedGraphBuilder::TryInlineApiCall(Handle function, CallOptimization optimization(function); if (!optimization.is_simple_api_call()) return false; Handle holder_map; - for (int i = 0; i < receiver_maps->length(); ++i) { - auto map = receiver_maps->at(i); - // Don't inline calls to receivers requiring accesschecks. - if (map->is_access_check_needed() && - map->instance_type() != JS_GLOBAL_PROXY_TYPE) { - return false; - } - } if (call_type == kCallApiFunction) { // Cannot embed a direct reference to the global proxy map // as it maybe dropped on deserialization. diff --git a/src/objects.cc b/src/objects.cc index 1cfdf63..f876cda 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -241,28 +241,21 @@ bool FunctionTemplateInfo::IsTemplateFor(Map* map) { // TODO(dcarney): CallOptimization duplicates this logic, merge. -Handle FunctionTemplateInfo::GetCompatibleReceiver( - Isolate* isolate, Handle receiver, bool is_construct) { +Object* FunctionTemplateInfo::GetCompatibleReceiver(Isolate* isolate, + Object* receiver) { // API calls are only supported with JSObject receivers. - if (!receiver->IsJSObject()) return isolate->factory()->null_value(); - auto js_receiver = Handle::cast(receiver); - if (!is_construct && js_receiver->IsAccessCheckNeeded() && - !isolate->MayAccess(js_receiver)) { - return isolate->factory()->null_value(); - } + if (!receiver->IsJSObject()) return isolate->heap()->null_value(); Object* recv_type = this->signature(); // No signature, return holder. if (recv_type->IsUndefined()) return receiver; FunctionTemplateInfo* signature = FunctionTemplateInfo::cast(recv_type); // Check the receiver. - for (PrototypeIterator iter(isolate, *receiver, + for (PrototypeIterator iter(isolate, receiver, PrototypeIterator::START_AT_RECEIVER); !iter.IsAtEnd(PrototypeIterator::END_AT_NON_HIDDEN); iter.Advance()) { - if (signature->IsTemplateFor(iter.GetCurrent())) { - return Handle(iter.GetCurrent(), isolate); - } + if (signature->IsTemplateFor(iter.GetCurrent())) return iter.GetCurrent(); } - return isolate->factory()->null_value(); + return isolate->heap()->null_value(); } diff --git a/src/objects.h b/src/objects.h index 0fc1d0d..cf9184f 100644 --- a/src/objects.h +++ b/src/objects.h @@ -10783,9 +10783,7 @@ class FunctionTemplateInfo: public TemplateInfo { // Returns the holder JSObject if the function can legally be called with this // receiver. Returns Heap::null_value() if the call is illegal. - Handle GetCompatibleReceiver(Isolate* isolate, - Handle receiver, - bool is_construct); + Object* GetCompatibleReceiver(Isolate* isolate, Object* receiver); private: // Bit position in the flag, from least significant bit position. diff --git a/test/cctest/test-accessors.cc b/test/cctest/test-accessors.cc index 5454699..bbb74c0 100644 --- a/test/cctest/test-accessors.cc +++ b/test/cctest/test-accessors.cc @@ -605,86 +605,3 @@ THREADED_TEST(Regress433458) { "Object.defineProperty(obj, 'prop', { writable: false });" "Object.defineProperty(obj, 'prop', { writable: true });"); } - - -static bool security_check_value = false; - - -static bool SecurityTestCallback(Local global, Local name, - v8::AccessType type, Local data) { - return security_check_value; -} - - -TEST(PrototypeGetterAccessCheck) { - i::FLAG_allow_natives_syntax = true; - LocalContext env; - v8::Isolate* isolate = env->GetIsolate(); - v8::HandleScope scope(isolate); - auto fun_templ = v8::FunctionTemplate::New(isolate); - auto getter_templ = v8::FunctionTemplate::New(isolate, handle_property); - getter_templ->SetLength(0); - fun_templ->InstanceTemplate()->SetAccessorProperty(v8_str("foo"), - getter_templ); - auto obj_templ = v8::ObjectTemplate::New(isolate); - obj_templ->SetAccessCheckCallbacks(SecurityTestCallback, nullptr); - env->Global()->Set(v8_str("Fun"), fun_templ->GetFunction()); - env->Global()->Set(v8_str("obj"), obj_templ->NewInstance()); - env->Global()->Set(v8_str("obj2"), obj_templ->NewInstance()); - - security_check_value = true; - CompileRun("var proto = new Fun();"); - CompileRun("obj.__proto__ = proto;"); - ExpectInt32("proto.foo", 907); - - // Test direct. - security_check_value = true; - ExpectInt32("obj.foo", 907); - security_check_value = false; - { - v8::TryCatch try_catch(isolate); - CompileRun("obj.foo"); - CHECK(try_catch.HasCaught()); - } - - // Test through call. - security_check_value = true; - ExpectInt32("proto.__lookupGetter__('foo').call(obj)", 907); - security_check_value = false; - { - v8::TryCatch try_catch(isolate); - CompileRun("proto.__lookupGetter__('foo').call(obj)"); - CHECK(try_catch.HasCaught()); - } - - // Test ics. - CompileRun( - "function f() {" - " var x;" - " for (var i = 0; i < 4; i++) {" - " x = obj.foo;" - " }" - " return x;" - "}"); - - security_check_value = true; - ExpectInt32("f()", 907); - security_check_value = false; - { - v8::TryCatch try_catch(isolate); - CompileRun("f();"); - CHECK(try_catch.HasCaught()); - } - - // Test crankshaft. - CompileRun("%OptimizeFunctionOnNextCall(f);"); - - security_check_value = true; - ExpectInt32("f()", 907); - security_check_value = false; - { - v8::TryCatch try_catch(isolate); - CompileRun("f();"); - CHECK(try_catch.HasCaught()); - } -} diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 3ad0e6c..5edbda7 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -8279,11 +8279,8 @@ TEST(DetachedAccesses) { CHECK(result.IsEmpty()); result = CompileRun("get_x_w()"); CHECK(result.IsEmpty()); - { - v8::TryCatch try_catch(env1->GetIsolate()); - CompileRun("this_x()"); - CHECK(try_catch.HasCaught()); - } + result = CompileRun("this_x()"); + CHECK(v8_str("env2_x")->Equals(result)); // Reattach env2's proxy env2 = Context::New(env1->GetIsolate(), -- 2.7.4