os,vm: fix segfaults and CHECK failure

Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: https://github.com/nodejs/node/issues/12369
Fixes: https://github.com/nodejs/node/issues/12370
PR-URL: https://github.com/nodejs/node/pull/12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>

src/node_contextify.cc

@@ -44,11 +44,13 @@ using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
+using v8::Just;
 using v8::Local;
 using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
+using v8::Nothing;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::Persistent;
@@ -547,17 +549,20 @@ class ContextifyScript : public BaseObject {
     Local<String> code = args[0]->ToString(env->isolate());
 
     Local<Value> options = args[1];
-    Local<String> filename = GetFilenameArg(env, options);
-    Local<Integer> lineOffset = GetLineOffsetArg(env, options);
-    Local<Integer> columnOffset = GetColumnOffsetArg(env, options);
-    bool display_errors = GetDisplayErrorsArg(env, options);
+    MaybeLocal<String> filename = GetFilenameArg(env, options);
+    MaybeLocal<Integer> lineOffset = GetLineOffsetArg(env, options);
+    MaybeLocal<Integer> columnOffset = GetColumnOffsetArg(env, options);
+    Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, options);
     MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, options);
-    bool produce_cached_data = GetProduceCachedData(env, options);
+    Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
     if (try_catch.HasCaught()) {
       try_catch.ReThrow();
       return;
     }
 
+    bool display_errors = maybe_display_errors.ToChecked();
+    bool produce_cached_data = maybe_produce_cached_data.ToChecked();
+
     ScriptCompiler::CachedData* cached_data = nullptr;
     Local<Uint8Array> ui8;
     if (cached_data_buf.ToLocal(&ui8)) {
@@ -567,7 +572,8 @@ class ContextifyScript : public BaseObject {
           ui8->ByteLength());
     }
 
-    ScriptOrigin origin(filename, lineOffset, columnOffset);
+    ScriptOrigin origin(filename.ToLocalChecked(), lineOffset.ToLocalChecked(),
+                        columnOffset.ToLocalChecked());
     ScriptCompiler::Source source(code, origin, cached_data);
     ScriptCompiler::CompileOptions compile_options =
         ScriptCompiler::kNoCompileOptions;
@@ -625,14 +631,18 @@ class ContextifyScript : public BaseObject {
 
     // Assemble arguments
     TryCatch try_catch(args.GetIsolate());
-    uint64_t timeout = GetTimeoutArg(env, args[0]);
-    bool display_errors = GetDisplayErrorsArg(env, args[0]);
-    bool break_on_sigint = GetBreakOnSigintArg(env, args[0]);
+    Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[0]);
+    Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[0]);
+    Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]);
     if (try_catch.HasCaught()) {
       try_catch.ReThrow();
       return;
     }
 
+    int64_t timeout = maybe_timeout.ToChecked();
+    bool display_errors = maybe_display_errors.ToChecked();
+    bool break_on_sigint = maybe_break_on_sigint.ToChecked();
+
     // Do the eval within this context
     EvalMachine(env, timeout, display_errors, break_on_sigint, args,
                 &try_catch);
@@ -655,13 +665,17 @@ class ContextifyScript : public BaseObject {
     Local<Object> sandbox = args[0].As<Object>();
     {
       TryCatch try_catch(env->isolate());
-      timeout = GetTimeoutArg(env, args[1]);
-      display_errors = GetDisplayErrorsArg(env, args[1]);
-      break_on_sigint = GetBreakOnSigintArg(env, args[1]);
+      Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[1]);
+      Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[1]);
+      Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]);
       if (try_catch.HasCaught()) {
         try_catch.ReThrow();
         return;
       }
+
+      timeout = maybe_timeout.ToChecked();
+      display_errors = maybe_display_errors.ToChecked();
+      break_on_sigint = maybe_break_on_sigint.ToChecked();
     }
 
     // Get the context from the sandbox
@@ -731,60 +745,82 @@ class ContextifyScript : public BaseObject {
         True(env->isolate()));
   }
 
-  static bool GetBreakOnSigintArg(Environment* env, Local<Value> options) {
+  static Maybe<bool> GetBreakOnSigintArg(Environment* env,
+                                         Local<Value> options) {
     if (options->IsUndefined() || options->IsString()) {
-      return false;
+      return Just(false);
     }
     if (!options->IsObject()) {
       env->ThrowTypeError("options must be an object");
-      return false;
+      return Nothing<bool>();
     }
 
     Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint");
-    Local<Value> value = options.As<Object>()->Get(key);
-    return value->IsTrue();
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), key);
+    if (maybe_value.IsEmpty())
+      return Nothing<bool>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
+    return Just(value->IsTrue());
   }
 
-  static int64_t GetTimeoutArg(Environment* env, Local<Value> options) {
+  static Maybe<int64_t> GetTimeoutArg(Environment* env, Local<Value> options) {
     if (options->IsUndefined() || options->IsString()) {
-      return -1;
+      return Just<int64_t>(-1);
     }
     if (!options->IsObject()) {
       env->ThrowTypeError("options must be an object");
-      return -1;
+      return Nothing<int64_t>();
     }
 
-    Local<Value> value = options.As<Object>()->Get(env->timeout_string());
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), env->timeout_string());
+    if (maybe_value.IsEmpty())
+      return Nothing<int64_t>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
     if (value->IsUndefined()) {
-      return -1;
+      return Just<int64_t>(-1);
     }
-    int64_t timeout = value->IntegerValue();
 
-    if (timeout <= 0) {
+    Maybe<int64_t> timeout = value->IntegerValue(env->context());
+
+    if (timeout.IsJust() && timeout.ToChecked() <= 0) {
       env->ThrowRangeError("timeout must be a positive number");
-      return -1;
+      return Nothing<int64_t>();
     }
+
     return timeout;
   }
 
 
-  static bool GetDisplayErrorsArg(Environment* env, Local<Value> options) {
+  static Maybe<bool> GetDisplayErrorsArg(Environment* env,
+                                         Local<Value> options) {
     if (options->IsUndefined() || options->IsString()) {
-      return true;
+      return Just(true);
     }
     if (!options->IsObject()) {
       env->ThrowTypeError("options must be an object");
-      return false;
+      return Nothing<bool>();
     }
 
     Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors");
-    Local<Value> value = options.As<Object>()->Get(key);
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), key);
+    if (maybe_value.IsEmpty())
+      return Nothing<bool>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
+    if (value->IsUndefined())
+      return Just(true);
 
-    return value->IsUndefined() ? true : value->BooleanValue();
+    return value->BooleanValue(env->context());
   }
 
 
-  static Local<String> GetFilenameArg(Environment* env, Local<Value> options) {
+  static MaybeLocal<String> GetFilenameArg(Environment* env,
+                                           Local<Value> options) {
     Local<String> defaultFilename =
         FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine.<anonymous>");
 
@@ -800,11 +836,15 @@ class ContextifyScript : public BaseObject {
     }
 
     Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename");
-    Local<Value> value = options.As<Object>()->Get(key);
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), key);
+    if (maybe_value.IsEmpty())
+      return MaybeLocal<String>();
 
+    Local<Value> value = maybe_value.ToLocalChecked();
     if (value->IsUndefined())
       return defaultFilename;
-    return value->ToString(env->isolate());
+    return value->ToString(env->context());
   }
 
 
@@ -813,7 +853,13 @@ class ContextifyScript : public BaseObject {
     if (!options->IsObject()) {
       return MaybeLocal<Uint8Array>();
     }
-    Local<Value> value = options.As<Object>()->Get(env->cached_data_string());
+
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), env->cached_data_string());
+    if (maybe_value.IsEmpty())
+      return MaybeLocal<Uint8Array>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
     if (value->IsUndefined()) {
       return MaybeLocal<Uint8Array>();
     }
@@ -827,18 +873,24 @@ class ContextifyScript : public BaseObject {
   }
 
 
-  static bool GetProduceCachedData(Environment* env, Local<Value> options) {
+  static Maybe<bool> GetProduceCachedData(Environment* env,
+                                          Local<Value> options) {
     if (!options->IsObject()) {
-      return false;
+      return Just(false);
     }
-    Local<Value> value =
-        options.As<Object>()->Get(env->produce_cached_data_string());
 
-    return value->IsTrue();
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(),
+                                  env->produce_cached_data_string());
+    if (maybe_value.IsEmpty())
+      return Nothing<bool>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
+    return Just(value->IsTrue());
   }
 
 
-  static Local<Integer> GetLineOffsetArg(Environment* env,
+  static MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
                                               Local<Value> options) {
     Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);
 
@@ -847,13 +899,20 @@ class ContextifyScript : public BaseObject {
     }
 
     Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset");
-    Local<Value> value = options.As<Object>()->Get(key);
+    MaybeLocal<Value> maybe_value =
+        options.As<Object>()->Get(env->context(), key);
+    if (maybe_value.IsEmpty())
+      return MaybeLocal<Integer>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
+    if (value->IsUndefined())
+      return defaultLineOffset;
 
-    return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
+    return value->ToInteger(env->context());
   }
 
 
-  static Local<Integer> GetColumnOffsetArg(Environment* env,
+  static MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
                                                 Local<Value> options) {
     Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);
 
@@ -862,9 +921,16 @@ class ContextifyScript : public BaseObject {
     }
 
     Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset");
-    Local<Value> value = options.As<Object>()->Get(key);
+    MaybeLocal<Value> maybe_value =
+      options.As<Object>()->Get(env->context(), key);
+    if (maybe_value.IsEmpty())
+      return MaybeLocal<Integer>();
+
+    Local<Value> value = maybe_value.ToLocalChecked();
+    if (value->IsUndefined())
+      return defaultColumnOffset;
 
-    return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
+    return value->ToInteger(env->context());
   }
 
 

src/node_os.cc

@@ -57,6 +57,7 @@ using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::Integer;
 using v8::Local;
+using v8::MaybeLocal;
 using v8::Null;
 using v8::Number;
 using v8::Object;
@@ -339,7 +340,12 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
 
   if (args[0]->IsObject()) {
     Local<Object> options = args[0].As<Object>();
-    Local<Value> encoding_opt = options->Get(env->encoding_string());
+    MaybeLocal<Value> maybe_encoding = options->Get(env->context(),
+                                                    env->encoding_string());
+    if (maybe_encoding.IsEmpty())
+      return;
+
+    Local<Value> encoding_opt = maybe_encoding.ToLocalChecked();
     encoding = ParseEncoding(env->isolate(), encoding_opt, UTF8);
   } else {
     encoding = UTF8;

test/parallel/test-regress-GH-12371.js

@@ -0,0 +1,37 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const execFile = require('child_process').execFile;
+
+const scripts = [
+  `os.userInfo({
+    get encoding() {
+      throw new Error('xyz');
+    }
+  })`
+];
+
+['filename', 'cachedData', 'produceCachedData', 'lineOffset', 'columnOffset']
+  .forEach((prop) => {
+    scripts.push(`vm.createScript('', {
+      get ${prop} () {
+        throw new Error('xyz');
+      }
+    })`);
+  });
+
+['breakOnSigint', 'timeout', 'displayErrors']
+  .forEach((prop) => {
+    scripts.push(`vm.createScript('').runInThisContext({
+      get ${prop} () {
+        throw new Error('xyz');
+      }
+    })`);
+  });
+
+scripts.forEach((script) => {
+  const node = process.execPath;
+  execFile(node, [ '-e', script ], common.mustCall((err, stdout, stderr) => {
+    assert(stderr.includes('Error: xyz'), 'createScript crashes');
+  }));
+});