From d5787278bce7d551a585da4b4006bf02b4c0eacf Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Wed, 11 Dec 2013 13:51:48 +0000 Subject: [PATCH] Fixed global object leak caused by overwriting the global receiver (the global proxy) in the global object with the global object itself. This CL additionally removes the API function to reattach a global proxy to a global object. BUG=324812 LOG=y R=dcarney@chromium.org, titzer@chromium.org Review URL: https://chromiumcodereview.appspot.com/101733002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18299 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- include/v8.h | 28 +++-------- src/api.cc | 16 +++---- src/bootstrapper.cc | 11 ----- src/bootstrapper.h | 3 -- src/objects-inl.h | 17 +++---- src/objects.h | 5 ++ src/runtime.cc | 10 ++++ src/runtime.h | 1 + src/v8natives.js | 7 ++- test/cctest/test-api.cc | 97 ++++++++++++++++++++++++-------------- test/cctest/test-decls.cc | 6 +-- test/cctest/test-object-observe.cc | 28 ++++------- 12 files changed, 113 insertions(+), 116 deletions(-) diff --git a/include/v8.h b/include/v8.h index 815f43d..9bf9608 100644 --- a/include/v8.h +++ b/include/v8.h @@ -5150,20 +5150,16 @@ class V8_EXPORT ExtensionConfiguration { class V8_EXPORT Context { public: /** - * Returns the global proxy object or global object itself for - * detached contexts. + * Returns the global proxy object. * - * Global proxy object is a thin wrapper whose prototype points to - * actual context's global object with the properties like Object, etc. - * This is done that way for security reasons (for more details see + * Global proxy object is a thin wrapper whose prototype points to actual + * context's global object with the properties like Object, etc. This is done + * that way for security reasons (for more details see * https://wiki.mozilla.org/Gecko:SplitWindow). * * Please note that changes to global proxy object prototype most probably - * would break VM---v8 expects only global object as a prototype of - * global proxy object. - * - * If DetachGlobal() has been invoked, Global() would return actual global - * object until global is reattached with ReattachGlobal(). + * would break VM---v8 expects only global object as a prototype of global + * proxy object. */ Local Global(); @@ -5174,18 +5170,6 @@ class V8_EXPORT Context { void DetachGlobal(); /** - * Reattaches a global object to a context. This can be used to - * restore the connection between a global object and a context - * after DetachGlobal has been called. - * - * \param global_object The global object to reattach to the - * context. For this to work, the global object must be the global - * object that was associated with this context before a call to - * DetachGlobal. - */ - void ReattachGlobal(Handle global_object); - - /** * Creates a new context and returns a handle to the newly allocated * context. * diff --git a/src/api.cc b/src/api.cc index 290fcba..f82ebe8 100644 --- a/src/api.cc +++ b/src/api.cc @@ -5409,6 +5409,12 @@ v8::Local Context::Global() { i::Handle context = Utils::OpenHandle(this); i::Isolate* isolate = context->GetIsolate(); i::Handle global(context->global_proxy(), isolate); + // TODO(dcarney): This should always return the global proxy + // but can't presently as calls to GetProtoype will return the wrong result. + if (i::Handle::cast( + global)->IsDetachedFrom(context->global_object())) { + global = i::Handle(context->global_object(), isolate); + } return Utils::ToLocal(i::Handle::cast(global)); } @@ -5421,16 +5427,6 @@ void Context::DetachGlobal() { } -void Context::ReattachGlobal(Handle global_object) { - i::Handle context = Utils::OpenHandle(this); - i::Isolate* isolate = context->GetIsolate(); - ENTER_V8(isolate); - i::Handle global_proxy = - i::Handle::cast(Utils::OpenHandle(*global_object)); - isolate->bootstrapper()->ReattachGlobal(context, global_proxy); -} - - void Context::AllowCodeGenerationFromStrings(bool allow) { i::Handle context = Utils::OpenHandle(this); i::Isolate* isolate = context->GetIsolate(); diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 1d2fb86..ac3fbb0 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -341,17 +341,6 @@ void Bootstrapper::DetachGlobal(Handle env) { Handle global_proxy(JSGlobalProxy::cast(env->global_proxy())); global_proxy->set_native_context(*factory->null_value()); SetObjectPrototype(global_proxy, factory->null_value()); - env->set_global_proxy(env->global_object()); - env->global_object()->set_global_receiver(env->global_object()); -} - - -void Bootstrapper::ReattachGlobal(Handle env, - Handle global_proxy) { - env->global_object()->set_global_receiver(*global_proxy); - env->set_global_proxy(*global_proxy); - SetObjectPrototype(global_proxy, Handle(env->global_object())); - global_proxy->set_native_context(*env); } diff --git a/src/bootstrapper.h b/src/bootstrapper.h index bac9f40..4f63c87 100644 --- a/src/bootstrapper.h +++ b/src/bootstrapper.h @@ -105,9 +105,6 @@ class Bootstrapper { // Detach the environment from its outer global object. void DetachGlobal(Handle env); - // Reattach an outer global object to an environment. - void ReattachGlobal(Handle env, Handle global_proxy); - // Traverses the pointers for memory management. void Iterate(ObjectVisitor* v); diff --git a/src/objects-inl.h b/src/objects-inl.h index 1717a5f..b41a8d3 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -5891,16 +5891,13 @@ PropertyAttributes JSReceiver::GetElementAttribute(uint32_t index) { } -// TODO(504): this may be useful in other places too where JSGlobalProxy -// is used. -Object* JSObject::BypassGlobalProxy() { - if (IsJSGlobalProxy()) { - Object* proto = GetPrototype(); - if (proto->IsNull()) return GetHeap()->undefined_value(); - ASSERT(proto->IsJSGlobalObject()); - return proto; - } - return this; +bool JSGlobalObject::IsDetached() { + return JSGlobalProxy::cast(global_receiver())->IsDetachedFrom(this); +} + + +bool JSGlobalProxy::IsDetachedFrom(GlobalObject* global) { + return GetPrototype() != global; } diff --git a/src/objects.h b/src/objects.h index cc77e19..7b14574 100644 --- a/src/objects.h +++ b/src/objects.h @@ -878,6 +878,7 @@ class DictionaryElementsAccessor; class ElementsAccessor; class Failure; class FixedArrayBase; +class GlobalObject; class ObjectVisitor; class StringStream; class Type; @@ -7412,6 +7413,8 @@ class JSGlobalProxy : public JSObject { // Casting. static inline JSGlobalProxy* cast(Object* obj); + inline bool IsDetachedFrom(GlobalObject* global); + // Dispatched behavior. DECLARE_PRINTER(JSGlobalProxy) DECLARE_VERIFIER(JSGlobalProxy) @@ -7481,6 +7484,8 @@ class JSGlobalObject: public GlobalObject { static Handle EnsurePropertyCell(Handle global, Handle name); + inline bool IsDetached(); + // Dispatched behavior. DECLARE_PRINTER(JSGlobalObject) DECLARE_VERIFIER(JSGlobalObject) diff --git a/src/runtime.cc b/src/runtime.cc index e763d43..8d84fda 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -9624,6 +9624,16 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GlobalReceiver) { } +RUNTIME_FUNCTION(MaybeObject*, Runtime_IsAttachedGlobal) { + SealHandleScope shs(isolate); + ASSERT(args.length() == 1); + Object* global = args[0]; + if (!global->IsJSGlobalObject()) return isolate->heap()->false_value(); + return isolate->heap()->ToBoolean( + !JSGlobalObject::cast(global)->IsDetached()); +} + + RUNTIME_FUNCTION(MaybeObject*, Runtime_ParseJson) { HandleScope scope(isolate); ASSERT_EQ(1, args.length()); diff --git a/src/runtime.h b/src/runtime.h index 95fb419..fe8a26b 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -275,6 +275,7 @@ namespace internal { \ /* Eval */ \ F(GlobalReceiver, 1, 1) \ + F(IsAttachedGlobal, 1, 1) \ F(ResolvePossiblyDirectEval, 5, 2) \ \ F(SetProperty, -1 /* 4 or 5 */, 1) \ diff --git a/src/v8natives.js b/src/v8natives.js index b715e89..96b88c5 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -170,19 +170,18 @@ function GlobalParseFloat(string) { function GlobalEval(x) { if (!IS_STRING(x)) return x; - var global_receiver = %GlobalReceiver(global); - var global_is_detached = (global === global_receiver); - // For consistency with JSC we require the global object passed to // eval to be the global object from which 'eval' originated. This // is not mandated by the spec. // We only throw if the global has been detached, since we need the // receiver as this-value for the call. - if (global_is_detached) { + if (!%IsAttachedGlobal(global)) { throw new $EvalError('The "this" value passed to eval must ' + 'be the global object from which eval originated'); } + var global_receiver = %GlobalReceiver(global); + var f = %CompileString(x, false); if (!IS_FUNCTION(f)) return f; diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index fdd79f1..91f3736 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -8527,8 +8527,6 @@ TEST(ContextDetachGlobal) { // Detach env2's global, and reuse the global object of env2 env2->Exit(); env2->DetachGlobal(); - // env2 has a new global object. - CHECK(!env2->Global()->Equals(global2)); v8::Handle env3 = Context::New(env1->GetIsolate(), 0, @@ -8563,7 +8561,7 @@ TEST(ContextDetachGlobal) { } -TEST(DetachAndReattachGlobal) { +TEST(DetachGlobal) { LocalContext env1; v8::HandleScope scope(env1->GetIsolate()); @@ -8628,16 +8626,58 @@ TEST(DetachAndReattachGlobal) { // so access should be blocked. result = CompileRun("other.p"); CHECK(result->IsUndefined()); +} - // Detach the global for env3 and reattach it to env2. - env3->DetachGlobal(); - env2->ReattachGlobal(global2); - // Check that we have access to other.p again in env1. |other| is now - // the global object for env2 which has the same security token as env1. - result = CompileRun("other.p"); - CHECK(result->IsInt32()); - CHECK_EQ(42, result->Int32Value()); +TEST(DetachedAccesses) { + LocalContext env1; + v8::HandleScope scope(env1->GetIsolate()); + + // Create second environment. + v8::Handle env2 = Context::New(env1->GetIsolate()); + + Local foo = v8_str("foo"); + + // Set same security token for env1 and env2. + env1->SetSecurityToken(foo); + env2->SetSecurityToken(foo); + + { + v8::Context::Scope scope(env2); + CompileRun( + "var x = 'x';" + "function get_x() { return this.x; }" + "function get_x_w() { return get_x(); }" + ""); + env1->Global()->Set(v8_str("get_x"), CompileRun("get_x")); + env1->Global()->Set(v8_str("get_x_w"), CompileRun("get_x_w")); + } + + Local env2_global = env2->Global(); + env2_global->TurnOnAccessCheck(); + env2->DetachGlobal(); + + Local result; + result = CompileRun("get_x()"); + CHECK(result->IsUndefined()); + result = CompileRun("get_x_w()"); + CHECK(result->IsUndefined()); + + // Reattach env2's proxy + env2 = Context::New(env1->GetIsolate(), + 0, + v8::Handle(), + env2_global); + env2->SetSecurityToken(foo); + { + v8::Context::Scope scope(env2); + CompileRun("var x = 'x2';"); + } + + result = CompileRun("get_x()"); + CHECK(result->IsUndefined()); + result = CompileRun("get_x_w()"); + CHECK_EQ(v8_str("x2"), result); } @@ -14251,8 +14291,10 @@ THREADED_TEST(TurnOnAccessCheck) { } // Detach the global and turn on access check. + Local hidden_global = Local::Cast( + context->Global()->GetPrototype()); context->DetachGlobal(); - context->Global()->TurnOnAccessCheck(); + hidden_global->TurnOnAccessCheck(); // Failing access check to property get results in undefined. CHECK(f1->Call(global, 0, NULL)->IsUndefined()); @@ -14336,8 +14378,10 @@ THREADED_TEST(TurnOnAccessCheckAndRecompile) { // Detach the global and turn on access check now blocking access to property // a and function h. + Local hidden_global = Local::Cast( + context->Global()->GetPrototype()); context->DetachGlobal(); - context->Global()->TurnOnAccessCheck(); + hidden_global->TurnOnAccessCheck(); // Failing access check to property get results in undefined. CHECK(f1->Call(global, 0, NULL)->IsUndefined()); @@ -14353,11 +14397,11 @@ THREADED_TEST(TurnOnAccessCheckAndRecompile) { // Now compile the source again. And get the newly compiled functions, except // for h for which access is blocked. CompileRun(source); - f1 = Local::Cast(context->Global()->Get(v8_str("f1"))); - f2 = Local::Cast(context->Global()->Get(v8_str("f2"))); - g1 = Local::Cast(context->Global()->Get(v8_str("g1"))); - g2 = Local::Cast(context->Global()->Get(v8_str("g2"))); - CHECK(context->Global()->Get(v8_str("h"))->IsUndefined()); + f1 = Local::Cast(hidden_global->Get(v8_str("f1"))); + f2 = Local::Cast(hidden_global->Get(v8_str("f2"))); + g1 = Local::Cast(hidden_global->Get(v8_str("g1"))); + g2 = Local::Cast(hidden_global->Get(v8_str("g2"))); + CHECK(hidden_global->Get(v8_str("h"))->IsUndefined()); // Failing access check to property get results in undefined. CHECK(f1->Call(global, 0, NULL)->IsUndefined()); @@ -15224,23 +15268,6 @@ THREADED_TEST(ReplaceConstantFunction) { } -// Regression test for http://crbug.com/16276. -THREADED_TEST(Regress16276) { - LocalContext context; - v8::HandleScope scope(context->GetIsolate()); - // Force the IC in f to be a dictionary load IC. - CompileRun("function f(obj) { return obj.x; }\n" - "var obj = { x: { foo: 42 }, y: 87 };\n" - "var x = obj.x;\n" - "delete obj.y;\n" - "for (var i = 0; i < 5; i++) f(obj);"); - // Detach the global object to make 'this' refer directly to the - // global object (not the proxy), and make sure that the dictionary - // load IC doesn't mess up loading directly from the global object. - context->DetachGlobal(); - CHECK_EQ(42, CompileRun("f(this).foo")->Int32Value()); -} - static void CheckElementValue(i::Isolate* isolate, int expected, i::Handle obj, diff --git a/test/cctest/test-decls.cc b/test/cctest/test-decls.cc index c3bd5c4..de0d745 100644 --- a/test/cctest/test-decls.cc +++ b/test/cctest/test-decls.cc @@ -648,9 +648,9 @@ class ExistsInHiddenPrototypeContext: public DeclarationContext { virtual void PostInitializeContext(Handle context) { Local global_object = context->Global(); Local hidden_proto = hidden_proto_->GetFunction()->NewInstance(); - context->DetachGlobal(); - context->Global()->SetPrototype(hidden_proto); - context->ReattachGlobal(global_object); + Local inner_global = + Local::Cast(global_object->GetPrototype()); + inner_global->SetPrototype(hidden_proto); } // Use the hidden prototype as the holder for the interceptors. diff --git a/test/cctest/test-object-observe.cc b/test/cctest/test-object-observe.cc index 4c32d44..b4e7c69 100644 --- a/test/cctest/test-object-observe.cc +++ b/test/cctest/test-object-observe.cc @@ -242,7 +242,6 @@ TEST(GlobalObjectObservation) { LocalContext context(isolate.GetIsolate()); HandleScope scope(isolate.GetIsolate()); Handle global_proxy = context->Global(); - Handle inner_global = global_proxy->GetPrototype().As(); CompileRun( "var records = [];" "var global = this;" @@ -255,33 +254,26 @@ TEST(GlobalObjectObservation) { context->DetachGlobal(); CompileRun("global.bar = 'goodbye';"); CHECK_EQ(1, CompileRun("records.length")->Int32Value()); - - // Mutating the global object directly still has an effect... - CompileRun("this.bar = 'goodbye';"); - CHECK_EQ(2, CompileRun("records.length")->Int32Value()); - CHECK(inner_global->StrictEquals(CompileRun("records[1].object"))); - - // Reattached, back to global proxy. - context->ReattachGlobal(global_proxy); - CompileRun("global.baz = 'again';"); - CHECK_EQ(3, CompileRun("records.length")->Int32Value()); - CHECK(global_proxy->StrictEquals(CompileRun("records[2].object"))); + CompileRun("this.baz = 'goodbye';"); + CHECK_EQ(1, CompileRun("records.length")->Int32Value()); // Attached to a different context, should not leak mutations // to the old context. context->DetachGlobal(); { LocalContext context2(isolate.GetIsolate()); - context2->DetachGlobal(); - context2->ReattachGlobal(global_proxy); CompileRun( "var records2 = [];" + "var global = this;" "Object.observe(this, function(r) { [].push.apply(records2, r) });" - "this.bat = 'context2';"); + "this.v1 = 'context2';"); + context2->DetachGlobal(); + CompileRun( + "global.v2 = 'context2';" + "this.v3 = 'context2';"); CHECK_EQ(1, CompileRun("records2.length")->Int32Value()); - CHECK(global_proxy->StrictEquals(CompileRun("records2[0].object"))); } - CHECK_EQ(3, CompileRun("records.length")->Int32Value()); + CHECK_EQ(1, CompileRun("records.length")->Int32Value()); // Attaching by passing to Context::New { @@ -295,7 +287,7 @@ TEST(GlobalObjectObservation) { CHECK_EQ(1, CompileRun("records3.length")->Int32Value()); CHECK(global_proxy->StrictEquals(CompileRun("records3[0].object"))); } - CHECK_EQ(3, CompileRun("records.length")->Int32Value()); + CHECK_EQ(1, CompileRun("records.length")->Int32Value()); } -- 2.7.4