vm: add code generation options #19016

devsnek · 2018-02-26T20:31:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

vm, src

addaleax · 2018-02-26T20:46:04Z

src/node_contextify.cc

@@ -184,6 +184,32 @@ Local<Context> ContextifyContext::CreateV8Context(
  CHECK(name->IsString());
  Utf8Value name_val(env->isolate(), name);

+  Local<Value> codegen = options_obj->Get(env->context(),
+      FIXED_ONE_BYTE_STRING(env->isolate(), "codeGeneration"))
+    .ToLocalChecked();


Can you use 4 spaces indentation for these lines as well, like other statement continuations?

addaleax · 2018-02-26T20:46:55Z

src/node_contextify.cc

+      .ToLocalChecked();
+    if (!allowwcg->IsUndefined()) {
+      CHECK(allowwcg->IsBoolean());
+      if(allowwcg.As<Boolean>()->Value() == false)


Style nit: space after if

addaleax · 2018-02-26T20:47:23Z

src/node_contextify.cc

+    if (!allowwcg->IsUndefined()) {
+      CHECK(allowwcg->IsBoolean());
+      if(allowwcg.As<Boolean>()->Value() == false)
+        ctx->SetEmbedderData(kAllowWasmCodeGeneration, True(env->isolate()));


Am I missing something? Shouldn’t this be true? (Or you could probably set the field directly to allowwcg)

addaleax · 2018-02-26T20:48:33Z

test/parallel/test-vm-codegen.js

+{
+  const ctx = createContext({}, { codeGeneration: {
+    strings: false,
+  } });


Style nit: Can you make codeGeneration have its own three lines?

addaleax · 2018-02-26T20:48:58Z

test/parallel/test-vm-codegen.js

+
+  const ctx = createContext({ bytes }, { codeGeneration: {
+    wasm: false,
+  } });


bnoordhuis · 2018-02-26T20:37:01Z

src/node.cc

+          CHECK(wcg->IsBoolean());
+          return wcg.As<Boolean>()->Value();
+        }
+        return true;


Could be just return wcg->IsTrue(); (also below.)

bnoordhuis · 2018-02-26T20:43:20Z

src/node_contextify.cc

+      CHECK(allowwcg->IsBoolean());
+      if(allowwcg.As<Boolean>()->Value() == false)
+        ctx->SetEmbedderData(kAllowWasmCodeGeneration, True(env->isolate()));
+    }


Apropos the variable names, allow_wasm_code_generation is clearer than allowwcg.

bnoordhuis · 2018-02-26T20:50:57Z

src/node_contextify.cc

+    CHECK(codegen->IsObject());
+    Local<Object> codegen_obj = codegen.As<Object>();
+
+    Local<Value> allowcgfs = codegen_obj->Get(env->context(),


Naming: allow_code_generation_from_strings

bnoordhuis · 2018-02-26T20:51:14Z

src/node_contextify.cc

+      ctx->AllowCodeGenerationFromStrings(allowcgfs.As<Boolean>()->Value());
+    }
+
+    Local<Value> allowwcg = codegen_obj->Get(env->context(),


Likewise: allow_wasm_code_generation

bnoordhuis · 2018-02-26T20:52:27Z

src/node.cc

@@ -4417,6 +4418,16 @@ inline int Start(uv_loop_t* event_loop,
  isolate->SetAbortOnUncaughtExceptionCallback(ShouldAbortOnUncaughtException);
  isolate->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);
  isolate->SetFatalErrorHandler(OnFatalError);
+  isolate->SetAllowWasmCodeGenerationCallback(
+      [](Local<Context> context, Local<String>) {


Make this a static member of ContextifyContext?

devsnek · 2018-02-26T22:48:02Z

CI ~~https://ci.nodejs.org/job/node-test-pull-request/13382/~~ https://ci.nodejs.org/job/node-test-pull-request/13385/

also i'm working on some docs now

devsnek · 2018-02-27T14:40:19Z

CI failures unrelated

@bnoordhuis comments addressed

bnoordhuis · 2018-02-27T22:10:48Z

src/node_contextify.cc

+    CHECK(wcg->IsBoolean());
+    return wcg.As<Boolean>()->Value();
+  }
+  return true;


Just return wcg->IsUndefined() || wcg->IsTrue();?

Aside: wasm_code_generation, or wasm_code_gen if you think that's too verbose, but no abbrevs.

bnoordhuis · 2018-02-27T22:11:21Z

src/node_contextify.cc

+    if (!allow_code_generation_from_strings->IsUndefined()) {
+      CHECK(allow_code_generation_from_strings->IsBoolean());
+      ctx->AllowCodeGenerationFromStrings(
+          allow_code_generation_from_strings.As<Boolean>()->Value());


Likewise. You don't even need the if.

bnoordhuis · 2018-02-27T22:13:09Z

src/node_contextify.cc

+      .ToLocalChecked();
+    if (!allow_wasm_code_generation->IsUndefined()) {
+      CHECK(allow_wasm_code_generation->IsBoolean());
+      ctx->SetEmbedderData(kAllowWasmCodeGeneration,


Better set it unconditionally because calling GetEmbedderData() on an undefined index is undefined behavior.

A better idea seems to be setting the slot to false unconditionally in NewContext() and then override the value in contextity, since the wasm callback is called on the main context as well.

i'll do tgu's approach

sidenote @nodejs/v8 is there any reason why we don't have Context::AllowWasmCodeGeneration/can it be added? it seems like the current approach here cause issues for people making native modules and creating a new context under node's isolate? what if someone takes over the callback without realizing that node uses it, etc.

cjihrig · 2018-02-28T14:21:32Z

test/parallel/test-vm-codegen.js

+    fn();
+    assert.fail('expected fn to error');
+  } catch (err) {
+    console.log(err);


If the console.log() is an intended part of the test, can you add a comment. Otherwise, please remove it.

cjihrig · 2018-02-28T14:25:03Z

test/parallel/test-vm-codegen.js

@@ -0,0 +1,70 @@
+'use strict';


This also needs tests for unexpected types of contextCodeGeneration and its properties.

TimothyGu · 2018-02-28T15:50:30Z

lib/vm.js

+      origin: options.contextOrigin,
+      codeGeneration:
+        validateObject(options.contextCodeGeneration,
+                       'options.contextCodeGeneration') ? {


The logic doesn’t currently work when options.contextCodeGeneration is undefined.

TimothyGu · 2018-03-01T00:34:11Z

lib/vm.js

 function getContextOptions(options) {
  if (options) {
    const contextOptions = {
      name: options.contextName,
-      origin: options.contextOrigin
+      origin: options.contextOrigin,
+      codeGeneration: options.contextCodeGeneration ? {


The typeof options.contextCodeGeneration === 'object' check should be kept though.

devsnek · 2018-03-01T01:28:53Z

CI https://ci.nodejs.org/job/node-test-pull-request/13416/

TimothyGu · 2018-03-01T05:04:03Z

lib/vm.js

+}
+
+function validateObject(prop, propName) {
+  if (prop !== undefined && prop !== null && typeof prop !== 'object')


Sorry for keeping going back and forth on this, but we don’t want to allow null here right?

yea that seems right. I think I accidentally inverted the standard object type check in my head which is why the null is there, I'll fix it

BridgeAR · 2018-03-01T21:34:49Z

lib/vm.js

+function validateObject(prop, propName) {
+  if (prop !== undefined && (prop === null || typeof prop !== 'object'))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', propName,
+                               'object', prop);


It should be 'Object'. I would also rather combine these validation functions instead of having one for each type.

how would i combine them

TimothyGu · 2018-03-04T19:11:02Z

lib/vm.js

+function validate(type, prop, propName) {
+  if (prop !== undefined &&
+      ((type === 'Object' && prop === null) ||
+      typeof prop !== type.toLowerCase()))


This logic is overly complicated, and would disallow V8 from inlining the typeof check. How it was (one validation function per type) was just fine.

TimothyGu · 2018-03-04T19:16:18Z

src/node.cc

@@ -4442,6 +4445,8 @@ inline int Start(uv_loop_t* event_loop,
  isolate->SetAbortOnUncaughtExceptionCallback(ShouldAbortOnUncaughtException);
  isolate->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);
  isolate->SetFatalErrorHandler(OnFatalError);
+  isolate->SetAllowWasmCodeGenerationCallback(
+      contextify::ContextifyContext::AllowWasmCodeGeneration);


Node.js core shouldn't have to depend on contextify. Please move AllowWasmCodeGeneration to node.cc.

@TimothyGu I think this was done by an explicit suggestion from @bnoordhuis … is there any specific reason not to do it this way?

Conceptually, AllowWasmCodeGeneration is used for all contexts, not just contexts created by contextify. It feels natural to move this function (which itself doesn't use any contextify-specific machinery) to node.cc.

Sorry for going back and forth on this, I didn't realize @bnoordhuis suggested this way.

i agree with timothy, but i'm going to wait for my other context pr to get merged before i work on this more

IIRC, when I made that suggestion, it was only setting it for ContextifyContext objects. It makes sense to move it back now.

TimothyGu · 2018-03-04T19:17:49Z

src/node.cc

@@ -4347,6 +4348,8 @@ Local<Context> NewContext(Isolate* isolate,
  HandleScope handle_scope(isolate);
  auto intl_key = FIXED_ONE_BYTE_STRING(isolate, "Intl");
  auto break_iter_key = FIXED_ONE_BYTE_STRING(isolate, "v8BreakIterator");
+  context->SetEmbedderData(
+      contextify::ContextifyContext::kAllowWasmCodeGeneration, True(isolate));


nit: move this to before intl_key declaration.

TimothyGu · 2018-03-04T19:21:33Z

src/node_contextify.h

  Environment* const env_;
  Persistent<v8::Context> context_;

 public:
+  // V8 reserves the first field in context objects for the debugger. We use the
+  // second field to hold a reference to the sandbox object.
+  enum { kSandboxObjectIndex = 1, kAllowWasmCodeGeneration = 2 };


Having the indices this way would make non-VM contexts have a hole at index 1, which doesn't sound too desirable. Also, ditto on the "core should not depend on contextify" comment here.

In other words:

Move kAllowWasmCodeGeneration to node_internals.h, and set it to 1. Keep kSandboxObjectIndex here however.

Make kSandboxObjectIndex equal to kAllowWasmCodeGeneration + 1.

You don't have to fix it now (though you certainly could), but "V8 reserves the first field in context objects for the debugger" is no longer true since v8/v8@5f90a6eb067. This means that kAllowWasmCodeGeneration could just be 0.

Oops, meant to click the "comment" button rather than "approve."

fhinkel · 2018-03-09T09:16:24Z

Can you please add a few paragraph to the commit message that explain this change. Thanks!

fhinkel · 2018-03-09T11:39:28Z

src/node_contextify.cc

+          .ToLocalChecked();
+
+  if (!codegen->IsUndefined()) {
+    CHECK(codegen->IsObject());


We could leave a comment here, that the js part already ensured that we have undefined or an object. Looking only at the C++ part, my first response was that this CHECK would fail a lot.

this pattern it used a lot with stuff that is validated js-side and we don't leave usually comments, are you worried about something specific not being construed?

fhinkel · 2018-03-09T11:45:45Z

lib/vm.js

+    throw new ERR_INVALID_ARG_TYPE(propName, 'boolean', prop);
+}
+
+function validateObject(prop, propName) {


Should we rather name this function validateObjectOrUndefined?

none of the others are named like that, and it seems fairly self-explanatory

Adds options to a VM Context to disable code generation from strings (such as eval or new Function) and WASM code generation (WebAssembly.compile).

devsnek · 2018-03-12T19:39:52Z

landed in cb5f358

Adds options to a VM Context to disable code generation from strings (such as eval or new Function) and WASM code generation (WebAssembly.compile). PR-URL: #19016 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

jdalton · 2018-03-13T23:07:51Z

Is there a feature request for this or a usage scenario?

devsnek · 2018-03-13T23:34:53Z

@jdalton i just have a general agenda of adding as much goodness to vm as possible; i really like vm :D

i also am using this in an eval bot i have in a discord support guild for javascript

targos · 2018-03-17T19:01:39Z

This is semver-minor, right?

targos · 2018-03-17T19:02:37Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

devsnek · 2018-03-17T20:04:36Z

looks like 9.x doesn't have v8::Isolate::SetAllowWasmCodeGenerationCallback, maybe it can be cherry picked in?

Add changes entries for vm.createContext codeGeneration option and script.runInNewContext contextCodeGeneration option. fixes: nodejs#19419 refs: nodejs#19016

gibfahn · 2018-03-19T08:35:33Z

Should be backported with #19440

Add changes entries for vm.createContext codeGeneration option and script.runInNewContext contextCodeGeneration option. PR-URL: #19440 Fixes: #19419 Refs: #19016 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Adds options to a VM Context to disable code generation from strings (such as eval or new Function) and WASM code generation (WebAssembly.compile). PR-URL: nodejs#19016 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2018

devsnek force-pushed the feature/vm-context-allowcgfs branch from 35f2f7c to ade9c38 Compare February 26, 2018 20:46

addaleax approved these changes Feb 26, 2018

View reviewed changes

bnoordhuis reviewed Feb 26, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from ade9c38 to afa8677 Compare February 26, 2018 22:46

devsnek force-pushed the feature/vm-context-allowcgfs branch from 83d67bf to b52b2e9 Compare February 26, 2018 23:42

TimothyGu added vm Issues and PRs related to the vm subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 27, 2018

bnoordhuis reviewed Feb 27, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from b52b2e9 to f765a8f Compare February 28, 2018 01:35

cjihrig reviewed Feb 28, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch 2 times, most recently from b9f0737 to c616b4a Compare February 28, 2018 15:36

TimothyGu reviewed Feb 28, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from c616b4a to 7327c16 Compare February 28, 2018 16:01

jasnell approved these changes Feb 28, 2018

View reviewed changes

TimothyGu reviewed Mar 1, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from 7327c16 to 83d5ebd Compare March 1, 2018 01:28

TimothyGu reviewed Mar 1, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from 83d5ebd to cbccf6c Compare March 1, 2018 15:10

BridgeAR reviewed Mar 1, 2018

View reviewed changes

devsnek force-pushed the feature/vm-context-allowcgfs branch from cbccf6c to 486ead3 Compare March 2, 2018 15:37

devsnek requested a review from TimothyGu March 4, 2018 01:52

TimothyGu previously approved these changes Mar 4, 2018

View reviewed changes

devsnek requested review from BridgeAR and TimothyGu March 9, 2018 01:03

fhinkel reviewed Mar 9, 2018

View reviewed changes

TimothyGu approved these changes Mar 9, 2018

View reviewed changes

vm: add code generation options

ecc0e20

Adds options to a VM Context to disable code generation from strings (such as eval or new Function) and WASM code generation (WebAssembly.compile).

devsnek force-pushed the feature/vm-context-allowcgfs branch from 43b83d8 to ecc0e20 Compare March 9, 2018 18:41

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2018

devsnek closed this Mar 12, 2018

devsnek deleted the feature/vm-context-allowcgfs branch March 12, 2018 19:39

targos added semver-minor PRs that contain new features and should be released in the next minor version. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 17, 2018

targos mentioned this pull request Mar 17, 2018

doc: missing "changes" entry for vm code generation options #19419

Closed

targos added the backport-requested-v9.x label Mar 17, 2018

TomCoded mentioned this pull request Mar 19, 2018

doc: add changes entry for vm code generation options #19440

Closed

2 tasks

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

vm: add code generation options #19016

vm: add code generation options #19016

Conversation

devsnek commented Feb 26, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Feb 26, 2018 • edited Loading

devsnek commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Mar 12, 2018

jdalton commented Mar 13, 2018

devsnek commented Mar 13, 2018 • edited Loading

targos commented Mar 17, 2018

targos commented Mar 17, 2018

devsnek commented Mar 17, 2018 • edited Loading

gibfahn commented Mar 19, 2018 • edited Loading

devsnek commented Feb 26, 2018 •

edited

Loading

devsnek commented Feb 26, 2018 •

edited

Loading

devsnek Feb 28, 2018 •

edited

Loading

devsnek commented Mar 13, 2018 •

edited

Loading

devsnek commented Mar 17, 2018 •

edited

Loading

gibfahn commented Mar 19, 2018 •

edited

Loading