diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 0d88a92eb41ecc..482414a5cf1f97 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1358,8 +1358,8 @@ E('ERR_VM_MODULE_NOT_MODULE', 'Provided module is not an instance of Module', Error); E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); E('ERR_WASI_ALREADY_STARTED', 'WASI instance has already started', Error); -E('ERR_WORKER_INVALID_EXEC_ARGV', (errors) => - `Initiated Worker with invalid execArgv flags: ${errors.join(', ')}`, +E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') => + `Initiated Worker with ${msg}: ${errors.join(', ')}`, Error); E('ERR_WORKER_NOT_RUNNING', 'Worker instance not running', Error); E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit', diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 683b1f6e6db8c7..953bc1d7368c2a 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -125,17 +125,16 @@ class Worker extends EventEmitter { const url = options.eval ? null : pathToFileURL(filename); // Set up the C++ handle for the worker, as well as some internal wiring. - this[kHandle] = new WorkerImpl(url, options.execArgv, + this[kHandle] = new WorkerImpl(url, + env === process.env ? null : env, + options.execArgv, parseResourceLimits(options.resourceLimits)); if (this[kHandle].invalidExecArgv) { throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv); } - if (env === process.env) { - // This may be faster than manually cloning the object in C++, especially - // when recursively spawning Workers. - this[kHandle].cloneParentEnvVars(); - } else if (env !== undefined) { - this[kHandle].setEnvVars(env); + if (this[kHandle].invalidNodeOptions) { + throw new ERR_WORKER_INVALID_EXEC_ARGV( + this[kHandle].invalidNodeOptions, 'invalid NODE_OPTIONS env variable'); } this[kHandle].onexit = (code, customErr) => this[kOnExit](code, customErr); this[kPort] = this[kHandle].messagePort; diff --git a/src/node.cc b/src/node.cc index acffc1cc119e90..a0398b1a4f8d2c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -790,80 +790,19 @@ int InitializeNodeWithArgs(std::vector* argv, V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1); #endif - std::shared_ptr default_env_options = - per_process::cli_options->per_isolate->per_env; - { - std::string text; - default_env_options->pending_deprecation = - credentials::SafeGetenv("NODE_PENDING_DEPRECATION", &text) && - text[0] == '1'; - } - - // Allow for environment set preserving symlinks. - { - std::string text; - default_env_options->preserve_symlinks = - credentials::SafeGetenv("NODE_PRESERVE_SYMLINKS", &text) && - text[0] == '1'; - } - - { - std::string text; - default_env_options->preserve_symlinks_main = - credentials::SafeGetenv("NODE_PRESERVE_SYMLINKS_MAIN", &text) && - text[0] == '1'; - } - - if (default_env_options->redirect_warnings.empty()) { - credentials::SafeGetenv("NODE_REDIRECT_WARNINGS", - &default_env_options->redirect_warnings); - } + HandleEnvOptions(per_process::cli_options->per_isolate->per_env); #if !defined(NODE_WITHOUT_NODE_OPTIONS) std::string node_options; if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) { - std::vector env_argv; - // [0] is expected to be the program name, fill it in from the real argv. - env_argv.push_back(argv->at(0)); - - bool is_in_string = false; - bool will_start_new_arg = true; - for (std::string::size_type index = 0; - index < node_options.size(); - ++index) { - char c = node_options.at(index); - - // Backslashes escape the following character - if (c == '\\' && is_in_string) { - if (index + 1 == node_options.size()) { - errors->push_back("invalid value for NODE_OPTIONS " - "(invalid escape)\n"); - return 9; - } else { - c = node_options.at(++index); - } - } else if (c == ' ' && !is_in_string) { - will_start_new_arg = true; - continue; - } else if (c == '"') { - is_in_string = !is_in_string; - continue; - } + std::vector env_argv = + ParseNodeOptionsEnvVar(node_options, errors); - if (will_start_new_arg) { - env_argv.emplace_back(std::string(1, c)); - will_start_new_arg = false; - } else { - env_argv.back() += c; - } - } + if (!errors->empty()) return 9; - if (is_in_string) { - errors->push_back("invalid value for NODE_OPTIONS " - "(unterminated string)\n"); - return 9; - } + // [0] is expected to be the program name, fill it in from the real argv. + env_argv.insert(env_argv.begin(), argv->at(0)); const int exit_code = ProcessGlobalArgs(&env_argv, nullptr, diff --git a/src/node_options-inl.h b/src/node_options-inl.h index 8bb5b6155a21d1..885cb849744205 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -3,9 +3,9 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include #include "node_options.h" #include "util.h" -#include namespace node { diff --git a/src/node_options.cc b/src/node_options.cc index 243e3a40cfaa4f..93f1e465e2610a 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -3,6 +3,7 @@ #include "env-inl.h" #include "node_binding.h" +#include "node_internals.h" #include #include @@ -1009,6 +1010,68 @@ void Initialize(Local target, } } // namespace options_parser + +void HandleEnvOptions(std::shared_ptr env_options) { + HandleEnvOptions(env_options, [](const char* name) { + std::string text; + return credentials::SafeGetenv(name, &text) ? text : ""; + }); +} + +void HandleEnvOptions(std::shared_ptr env_options, + std::function opt_getter) { + env_options->pending_deprecation = + opt_getter("NODE_PENDING_DEPRECATION") == "1"; + + env_options->preserve_symlinks = opt_getter("NODE_PRESERVE_SYMLINKS") == "1"; + + env_options->preserve_symlinks_main = + opt_getter("NODE_PRESERVE_SYMLINKS_MAIN") == "1"; + + if (env_options->redirect_warnings.empty()) + env_options->redirect_warnings = opt_getter("NODE_REDIRECT_WARNINGS"); +} + +std::vector ParseNodeOptionsEnvVar( + const std::string& node_options, std::vector* errors) { + std::vector env_argv; + + bool is_in_string = false; + bool will_start_new_arg = true; + for (std::string::size_type index = 0; index < node_options.size(); ++index) { + char c = node_options.at(index); + + // Backslashes escape the following character + if (c == '\\' && is_in_string) { + if (index + 1 == node_options.size()) { + errors->push_back("invalid value for NODE_OPTIONS " + "(invalid escape)\n"); + return env_argv; + } else { + c = node_options.at(++index); + } + } else if (c == ' ' && !is_in_string) { + will_start_new_arg = true; + continue; + } else if (c == '"') { + is_in_string = !is_in_string; + continue; + } + + if (will_start_new_arg) { + env_argv.emplace_back(std::string(1, c)); + will_start_new_arg = false; + } else { + env_argv.back() += c; + } + } + + if (is_in_string) { + errors->push_back("invalid value for NODE_OPTIONS " + "(unterminated string)\n"); + } + return env_argv; +} } // namespace node NODE_MODULE_CONTEXT_AWARE_INTERNAL(options, node::options_parser::Initialize) diff --git a/src/node_options.h b/src/node_options.h index f49d35cedad058..470007f06b6342 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -457,6 +457,13 @@ extern Mutex cli_options_mutex; extern std::shared_ptr cli_options; } // namespace per_process + +void HandleEnvOptions(std::shared_ptr env_options); +void HandleEnvOptions(std::shared_ptr env_options, + std::function opt_getter); + +std::vector ParseNodeOptionsEnvVar( + const std::string& node_options, std::vector* errors); } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_worker.cc b/src/node_worker.cc index e4c5cd42abf1a2..47eb813d5e2c24 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -16,6 +16,7 @@ #include #include +using node::kAllowedInEnvironment; using node::kDisallowedInEnvironment; using v8::Array; using v8::ArrayBuffer; @@ -47,14 +48,15 @@ Worker::Worker(Environment* env, Local wrap, const std::string& url, std::shared_ptr per_isolate_opts, - std::vector&& exec_argv) + std::vector&& exec_argv, + std::shared_ptr env_vars) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), per_isolate_opts_(per_isolate_opts), exec_argv_(exec_argv), platform_(env->isolate_data()->platform()), start_profiler_idle_notifier_(env->profiler_idle_notifier_started()), thread_id_(Environment::AllocateThreadId()), - env_vars_(env->env_vars()) { + env_vars_(env_vars) { Debug(this, "Creating new worker instance with thread id %llu", thread_id_); // Set up everything that needs to be set up in the parent environment. @@ -442,6 +444,7 @@ Worker::~Worker() { void Worker::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Isolate* isolate = args.GetIsolate(); CHECK(args.IsConstructCall()); @@ -452,24 +455,81 @@ void Worker::New(const FunctionCallbackInfo& args) { std::string url; std::shared_ptr per_isolate_opts = nullptr; + std::shared_ptr env_vars = nullptr; std::vector exec_argv_out; - bool has_explicit_exec_argv = false; - CHECK_EQ(args.Length(), 3); + CHECK_EQ(args.Length(), 4); // Argument might be a string or URL if (!args[0]->IsNullOrUndefined()) { Utf8Value value( - args.GetIsolate(), - args[0]->ToString(env->context()).FromMaybe(Local())); + isolate, args[0]->ToString(env->context()).FromMaybe(Local())); url.append(value.out(), value.length()); } - if (args[1]->IsArray()) { - Local array = args[1].As(); + if (args[1]->IsNull()) { + // Means worker.env = { ...process.env }. + env_vars = env->env_vars()->Clone(isolate); + } else if (args[1]->IsObject()) { + // User provided env. + env_vars = KVStore::CreateMapKVStore(); + env_vars->AssignFromObject(isolate->GetCurrentContext(), + args[1].As()); + } else { + // Env is shared. + env_vars = env->env_vars(); + } + + if (args[1]->IsObject() || args[2]->IsArray()) { + per_isolate_opts.reset(new PerIsolateOptions()); + + HandleEnvOptions( + per_isolate_opts->per_env, [isolate, &env_vars](const char* name) { + MaybeLocal value = + env_vars->Get(isolate, OneByteString(isolate, name)); + return value.IsEmpty() ? std::string{} + : std::string(*String::Utf8Value( + isolate, value.ToLocalChecked())); + }); + +#ifndef NODE_WITHOUT_NODE_OPTIONS + MaybeLocal maybe_node_opts = + env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS")); + if (!maybe_node_opts.IsEmpty()) { + std::string node_options( + *String::Utf8Value(isolate, maybe_node_opts.ToLocalChecked())); + std::vector errors{}; + std::vector env_argv = + ParseNodeOptionsEnvVar(node_options, &errors); + // [0] is expected to be the program name, add dummy string. + env_argv.insert(env_argv.begin(), ""); + std::vector invalid_args{}; + options_parser::Parse(&env_argv, + nullptr, + &invalid_args, + per_isolate_opts.get(), + kAllowedInEnvironment, + &errors); + if (errors.size() > 0 && args[1]->IsObject()) { + // Only fail for explicitly provided env, this protects from failures + // when NODE_OPTIONS from parent's env is used (which is the default). + Local error; + if (!ToV8Value(env->context(), errors).ToLocal(&error)) return; + Local key = + FIXED_ONE_BYTE_STRING(env->isolate(), "invalidNodeOptions"); + // Ignore the return value of Set() because exceptions bubble up to JS + // when we return anyway. + USE(args.This()->Set(env->context(), key, error)); + return; + } + } +#endif + } + + if (args[2]->IsArray()) { + Local array = args[2].As(); // The first argument is reserved for program name, but we don't need it // in workers. - has_explicit_exec_argv = true; std::vector exec_argv = {""}; uint32_t length = array->Length(); for (uint32_t i = 0; i < length; i++) { @@ -491,8 +551,6 @@ void Worker::New(const FunctionCallbackInfo& args) { std::vector invalid_args{}; std::vector errors{}; - per_isolate_opts.reset(new PerIsolateOptions()); - // Using invalid_args as the v8_args argument as it stores unknown // options for the per isolate parser. options_parser::Parse( @@ -519,40 +577,24 @@ void Worker::New(const FunctionCallbackInfo& args) { USE(args.This()->Set(env->context(), key, error)); return; } - } - if (!has_explicit_exec_argv) + } else { exec_argv_out = env->exec_argv(); + } - Worker* worker = - new Worker(env, args.This(), url, per_isolate_opts, - std::move(exec_argv_out)); + Worker* worker = new Worker(env, + args.This(), + url, + per_isolate_opts, + std::move(exec_argv_out), + env_vars); - CHECK(args[2]->IsFloat64Array()); - Local limit_info = args[2].As(); + CHECK(args[3]->IsFloat64Array()); + Local limit_info = args[3].As(); CHECK_EQ(limit_info->Length(), kTotalResourceLimitCount); limit_info->CopyContents(worker->resource_limits_, sizeof(worker->resource_limits_)); } -void Worker::CloneParentEnvVars(const FunctionCallbackInfo& args) { - Worker* w; - ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); - CHECK(w->thread_joined_); // The Worker has not started yet. - - w->env_vars_ = w->env()->env_vars()->Clone(args.GetIsolate()); -} - -void Worker::SetEnvVars(const FunctionCallbackInfo& args) { - Worker* w; - ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); - CHECK(w->thread_joined_); // The Worker has not started yet. - - CHECK(args[0]->IsObject()); - w->env_vars_ = KVStore::CreateMapKVStore(); - w->env_vars_->AssignFromObject(args.GetIsolate()->GetCurrentContext(), - args[0].As()); -} - void Worker::StartThread(const FunctionCallbackInfo& args) { Worker* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); @@ -724,8 +766,6 @@ void InitWorker(Local target, w->InstanceTemplate()->SetInternalFieldCount(1); w->Inherit(AsyncWrap::GetConstructorTemplate(env)); - env->SetProtoMethod(w, "setEnvVars", Worker::SetEnvVars); - env->SetProtoMethod(w, "cloneParentEnvVars", Worker::CloneParentEnvVars); env->SetProtoMethod(w, "startThread", Worker::StartThread); env->SetProtoMethod(w, "stopThread", Worker::StopThread); env->SetProtoMethod(w, "ref", Worker::Ref); diff --git a/src/node_worker.h b/src/node_worker.h index 744c31a9932174..0c6fd35c0ab032 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -26,7 +26,8 @@ class Worker : public AsyncWrap { v8::Local wrap, const std::string& url, std::shared_ptr per_isolate_opts, - std::vector&& exec_argv); + std::vector&& exec_argv, + std::shared_ptr env_vars); ~Worker() override; // Run the worker. This is only called from the worker thread. diff --git a/test/parallel/test-cli-node-options.js b/test/parallel/test-cli-node-options.js index 698f463b627644..51a0d9a952e614 100644 --- a/test/parallel/test-cli-node-options.js +++ b/test/parallel/test-cli-node-options.js @@ -7,6 +7,7 @@ if (process.config.variables.node_without_node_options) const assert = require('assert'); const exec = require('child_process').execFile; +const { Worker } = require('worker_threads'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); @@ -14,13 +15,13 @@ tmpdir.refresh(); const printA = require.resolve('../fixtures/printA.js'); const printSpaceA = require.resolve('../fixtures/print A.js'); -expect(` -r ${printA} `, 'A\nB\n'); -expect(`-r ${printA}`, 'A\nB\n'); -expect(`-r ${JSON.stringify(printA)}`, 'A\nB\n'); -expect(`-r ${JSON.stringify(printSpaceA)}`, 'A\nB\n'); -expect(`-r ${printA} -r ${printA}`, 'A\nB\n'); -expect(` -r ${printA} -r ${printA}`, 'A\nB\n'); -expect(` --require ${printA} --require ${printA}`, 'A\nB\n'); +expectNoWorker(` -r ${printA} `, 'A\nB\n'); +expectNoWorker(`-r ${printA}`, 'A\nB\n'); +expectNoWorker(`-r ${JSON.stringify(printA)}`, 'A\nB\n'); +expectNoWorker(`-r ${JSON.stringify(printSpaceA)}`, 'A\nB\n'); +expectNoWorker(`-r ${printA} -r ${printA}`, 'A\nB\n'); +expectNoWorker(` -r ${printA} -r ${printA}`, 'A\nB\n'); +expectNoWorker(` --require ${printA} --require ${printA}`, 'A\nB\n'); expect('--no-deprecation', 'B\n'); expect('--no-warnings', 'B\n'); expect('--no_warnings', 'B\n'); @@ -28,16 +29,22 @@ expect('--trace-warnings', 'B\n'); expect('--redirect-warnings=_', 'B\n'); expect('--trace-deprecation', 'B\n'); expect('--trace-sync-io', 'B\n'); -expect('--trace-events-enabled', 'B\n'); +expectNoWorker('--trace-events-enabled', 'B\n'); expect('--track-heap-objects', 'B\n'); -expect('--throw-deprecation', 'B\n'); -expect('--zero-fill-buffers', 'B\n'); -expect('--v8-pool-size=10', 'B\n'); -expect('--trace-event-categories node', 'B\n'); -// eslint-disable-next-line no-template-curly-in-string -expect('--trace-event-file-pattern {pid}-${rotation}.trace_events', 'B\n'); +expect('--throw-deprecation', + /.*DeprecationWarning: Buffer\(\) is deprecated due to security and usability issues.*/, + 'new Buffer(42)', + true); +expectNoWorker('--zero-fill-buffers', 'B\n'); +expectNoWorker('--v8-pool-size=10', 'B\n'); +expectNoWorker('--trace-event-categories node', 'B\n'); +expectNoWorker( + // eslint-disable-next-line no-template-curly-in-string + '--trace-event-file-pattern {pid}-${rotation}.trace_events', + 'B\n' +); // eslint-disable-next-line no-template-curly-in-string -expect('--trace-event-file-pattern {pid}-${rotation}.trace_events ' + +expectNoWorker('--trace-event-file-pattern {pid}-${rotation}.trace_events ' + '--trace-event-categories node.async_hooks', 'B\n'); expect('--unhandled-rejections=none', 'B\n'); @@ -53,9 +60,9 @@ if (common.isLinux && ['arm', 'x64'].includes(process.arch)) { } if (common.hasCrypto) { - expect('--use-openssl-ca', 'B\n'); - expect('--use-bundled-ca', 'B\n'); - expect('--openssl-config=_ossl_cfg', 'B\n'); + expectNoWorker('--use-openssl-ca', 'B\n'); + expectNoWorker('--use-bundled-ca', 'B\n'); + expectNoWorker('--openssl-config=_ossl_cfg', 'B\n'); } // V8 options @@ -63,14 +70,20 @@ expect('--abort_on-uncaught_exception', 'B\n'); expect('--disallow-code-generation-from-strings', 'B\n'); expect('--max-old-space-size=0', 'B\n'); expect('--stack-trace-limit=100', - /(\s*at f \(\[eval\]:1:\d*\)\r?\n){100}/, + /(\s*at f \(\[(eval|worker eval)\]:1:\d*\)\r?\n)/, '(function f() { f(); })();', true); // Unsupported on arm. See https://crbug.com/v8/8713. if (!['arm', 'arm64'].includes(process.arch)) expect('--interpreted-frames-native-stack', 'B\n'); -function expect(opt, want, command = 'console.log("B")', wantsError = false) { +function expectNoWorker(opt, want, command, wantsError) { + expect(opt, want, command, wantsError, false); +} + +function expect( + opt, want, command = 'console.log("B")', wantsError = false, testWorker = true +) { const argv = ['-e', command]; const opts = { cwd: tmpdir.path, @@ -79,15 +92,52 @@ function expect(opt, want, command = 'console.log("B")', wantsError = false) { }; if (typeof want === 'string') want = new RegExp(want); - exec(process.execPath, argv, opts, common.mustCall((err, stdout, stderr) => { + + const test = (type) => common.mustCall((err, stdout) => { + const o = JSON.stringify(opt); if (wantsError) { - stdout = stderr; + assert.ok(err, `${type}: expected error for ${o}`); + stdout = err.stack; } else { - assert.ifError(err); + assert.ifError(err, `${type}: failed for ${o}`); } if (want.test(stdout)) return; - const o = JSON.stringify(opt); - assert.fail(`For ${o}, failed to find ${want} in: <\n${stdout}\n>`); + assert.fail( + `${type}: for ${o}, failed to find ${want} in: <\n${stdout}\n>` + ); + }); + + exec(process.execPath, argv, opts, test('child process')); + if (testWorker) + workerTest(opts, command, wantsError, test('worker')); +} + +async function collectStream(readable) { + readable.setEncoding('utf8'); + let data = ''; + for await (const chunk of readable) { + data += chunk; + } + return data; +} + +function workerTest(opts, command, wantsError, test) { + let workerError = null; + const worker = new Worker(command, { + ...opts, + execArgv: [], + eval: true, + stdout: true, + stderr: true + }); + worker.on('error', (err) => { + workerError = err; + }); + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, wantsError ? 1 : 0); + collectStream(worker.stdout).then((stdout) => { + test(workerError, stdout); + }); })); } diff --git a/test/parallel/test-require-symlink.js b/test/parallel/test-require-symlink.js index 2e8a03ac820df5..14eefcd60d32dc 100644 --- a/test/parallel/test-require-symlink.js +++ b/test/parallel/test-require-symlink.js @@ -12,6 +12,7 @@ const { spawn } = require('child_process'); const fs = require('fs'); const path = require('path'); const process = require('process'); +const { Worker } = require('worker_threads'); // Setup: Copy fixtures to tmp directory. @@ -53,7 +54,7 @@ const linkDir = path.join(dirName, const linkTarget = path.join('..', '..', 'dep2'); -const linkScript = 'linkscript.js'; +const linkScript = './linkscript.js'; const linkScriptTarget = path.join(dirName, 'symlinked.js'); @@ -84,4 +85,31 @@ function test() { assert.strictEqual(code, 0); assert(!signal); }); + + // Also verify that symlinks works for setting preserve via env variables in + // Workers. + const worker = new Worker(linkScript, { + env: { ...process.env, NODE_PRESERVE_SYMLINKS: '1' } + }); + worker.on('error', (err) => { + console.log('Worker failed'); + throw err; + }); + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); + + // Also verify that symlinks works for setting preserve via env variables in + // Workers with explicit execArgv. + const workerArgv = new Worker(linkScript, { + execArgv: [], + env: { ...process.env, NODE_PRESERVE_SYMLINKS: '1' } + }); + workerArgv.on('error', (err) => { + console.log('Worker with execArgv failed'); + throw err; + }); + workerArgv.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); }