contextify: tie lifetimes of context & sandbox

When the previous set of changes (bfff07b) it was possible to have the context get garbage collected while sandbox was still live. We need to tie the lifetime of the context to the lifetime of the sandbox. Fixes: #5768 PR-URL: #5786 Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: cjihrig - Colin Ihrig <[email protected]> Reviewed-By: trevnorris - Trevor Norris <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
nodejs · Mar 19, 2016 · a53b2ac · a53b2ac
1 parent be97db9
commit a53b2ac
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 5 deletions.
diff --git a/src/env.h b/src/env.h
@@ -50,7 +50,8 @@ namespace node {
 #define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)                              \
   V(alpn_buffer_private_symbol, "node:alpnBuffer")                            \
   V(arrow_message_private_symbol, "node:arrowMessage")                        \
-  V(contextify_private_symbol, "node:contextify")                             \
+  V(contextify_context_private_symbol, "node:contextify:context")             \
+  V(contextify_global_private_symbol, "node:contextify:global")               \
   V(decorated_private_symbol, "node:decorated")                               \
   V(npn_buffer_private_symbol, "node:npnBuffer")                              \
   V(processed_private_symbol, "node:processed")                               \

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -209,7 +209,17 @@ class ContextifyContext {
 
     CHECK(!ctx.IsEmpty());
     ctx->SetSecurityToken(env->context()->GetSecurityToken());
+
+    // We need to tie the lifetime of the sandbox object with the lifetime of
+    // newly created context. We do this by making them hold references to each
+    // other. The context can directly hold a reference to the sandbox as an
+    // embedder data field. However, we cannot hold a reference to a v8::Context
+    // directly in an Object, we instead hold onto the new context's global
+    // object instead (which then has a reference to the context).
     ctx->SetEmbedderData(kSandboxObjectIndex, sandbox_obj);
+    sandbox_obj->SetPrivate(env->context(),
+                            env->contextify_global_private_symbol(),
+                            ctx->Global());
 
     env->AssignToContext(ctx);
 
@@ -270,7 +280,7 @@ class ContextifyContext {
     CHECK(
         !sandbox->HasPrivate(
             env->context(),
-            env->contextify_private_symbol()).FromJust());
+            env->contextify_context_private_symbol()).FromJust());
 
     TryCatch try_catch(env->isolate());
     ContextifyContext* context = new ContextifyContext(env, sandbox);
@@ -285,7 +295,7 @@ class ContextifyContext {
 
     sandbox->SetPrivate(
         env->context(),
-        env->contextify_private_symbol(),
+        env->contextify_context_private_symbol(),
         External::New(env->isolate(), context));
   }
 
@@ -300,7 +310,8 @@ class ContextifyContext {
     Local<Object> sandbox = args[0].As<Object>();
 
     auto result =
-        sandbox->HasPrivate(env->context(), env->contextify_private_symbol());
+        sandbox->HasPrivate(env->context(),
+                            env->contextify_context_private_symbol());
     args.GetReturnValue().Set(result.FromJust());
   }
 
@@ -315,7 +326,8 @@ class ContextifyContext {
       Environment* env,
       const Local<Object>& sandbox) {
     auto maybe_value =
-        sandbox->GetPrivate(env->context(), env->contextify_private_symbol());
+        sandbox->GetPrivate(env->context(),
+                            env->contextify_context_private_symbol());
     Local<Value> context_external_v;
     if (maybe_value.ToLocal(&context_external_v) &&
         context_external_v->IsExternal()) {

diff --git a/test/parallel/test-vm-create-and-run-in-context.js b/test/parallel/test-vm-create-and-run-in-context.js
@@ -1,4 +1,5 @@
 'use strict';
+// Flags: --expose-gc
 require('../common');
 var assert = require('assert');
 
@@ -18,3 +19,11 @@ console.error('test updating context');
 result = vm.runInContext('var foo = 3;', context);
 assert.equal(3, context.foo);
 assert.equal('lala', context.thing);
+
+// https://github.com/nodejs/node/issues/5768
+console.error('run in contextified sandbox without referencing the context');
+var sandbox = {x: 1};
+vm.createContext(sandbox);
+gc();
+vm.runInContext('x = 2', sandbox);
+// Should not crash.