From a53b2ac4b1dcde5579d9cba814ca0c4b78e59a9f Mon Sep 17 00:00:00 2001
From: Ali Ijaz Sheikh <ofrobots@google.com>
Date: Fri, 18 Mar 2016 08:32:33 -0700
Subject: [PATCH] contextify: tie lifetimes of context & sandbox

When the previous set of changes (bfff07b4) 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: https://github.com/nodejs/node/issues/5768
PR-URL: https://github.com/nodejs/node/pull/5786
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
---
 src/env.h                                     |  3 ++-
 src/node_contextify.cc                        | 20 +++++++++++++++----
 .../test-vm-create-and-run-in-context.js      |  9 +++++++++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/env.h b/src/env.h
index c768120ab83d8b..1e3746b2633a96 100644
--- 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
index 6e04d5836ff894..48709947893621 100644
--- 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
index 94527598ca27b1..15efc8f527b8c9 100644
--- 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.