From bc8940785a1e35ded02af7d64a0ee8e47fba616c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 4 Dec 2016 10:38:35 -0800 Subject: [PATCH 1/9] process: add --redirect-warnings command line argument The --redirect-warnings command line argument allows process warnings to be written to a specified file rather than printed to stderr. Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable. If the specified file cannot be opened or written to for any reason, the argument is ignored and the warning is printed to stderr. If the file already exists, it will be appended to. PR-URL: https://github.com/nodejs/node/pull/10116 Reviewed-By: Michael Dawson Reviewed-By: Michal Zasso Reviewed-By: Fedor Indutny --- doc/api/cli.md | 21 ++++++ doc/node.1 | 10 +++ lib/internal/process/warning.js | 75 ++++++++++++++++++- src/node.cc | 13 ++++ src/node_config.cc | 10 +++ src/node_internals.h | 5 ++ .../test-process-redirect-warnings-env.js | 25 +++++++ .../test-process-redirect-warnings.js | 25 +++++++ 8 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-process-redirect-warnings-env.js create mode 100644 test/parallel/test-process-redirect-warnings.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 650e0cdbdb607e..09fabb77334ff3 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -122,6 +122,16 @@ added: v6.0.0 Print stack traces for process warnings (including deprecations). +### `--redirect-warnings=file` + + +Write process warnings to the given file instead of printing to stderr. The +file will be created if it does not exist, and will be appended to if it does. +If an error occurs while attempting to write the warning to the file, the +warning will be written to stderr instead. + ### `--trace-sync-io` + +When set, process warnings will be emitted to the given file instead of +printing to stderr. The file will be created if it does not exist, and will be +appended to if it does. If an error occurs while attempting to write the +warning to the file, the warning will be written to stderr instead. This is +equivalent to using the `--redirect-warnings=file` command-line flag. + [emit_warning]: process.html#process_process_emitwarning_warning_name_ctor [Buffer]: buffer.html#buffer_buffer [debugger]: debugger.html diff --git a/doc/node.1 b/doc/node.1 index 2167f245216e04..469735eebae9bf 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -112,6 +112,10 @@ Silence all process warnings (including deprecations). .BR \-\-trace\-warnings Print stack traces for process warnings (including deprecations). +.TP +.BR \-\-redirect\-warnings=\fIfile\fR +Write process warnings to the given file instead of printing to stderr. + .TP .BR \-\-trace\-sync\-io Print a stack trace whenever synchronous I/O is detected after the first turn @@ -255,6 +259,12 @@ containing trusted certificates. If \fB\-\-use\-openssl\-ca\fR is enabled, this overrides and sets OpenSSL's file containing trusted certificates. +.TP +.BR NODE_REDIRECT_WARNINGS=\fIfile\fR +Write process warnings to the given file instead of printing to stderr. +(equivalent to using the \-\-redirect\-warnings=\fIfile\fR command-line +argument). + .SH BUGS Bugs are tracked in GitHub Issues: .ur https://github.com/nodejs/node/issues diff --git a/lib/internal/process/warning.js b/lib/internal/process/warning.js index bce470cd1c50ac..dd9aa484b7ea21 100644 --- a/lib/internal/process/warning.js +++ b/lib/internal/process/warning.js @@ -4,10 +4,81 @@ const traceWarnings = process.traceProcessWarnings; const noDeprecation = process.noDeprecation; const traceDeprecation = process.traceDeprecation; const throwDeprecation = process.throwDeprecation; +const config = process.binding('config'); const prefix = `(${process.release.name}:${process.pid}) `; exports.setup = setupProcessWarnings; +var fs; +var cachedFd; +var acquiringFd = false; +function nop() {} + +function lazyFs() { + if (!fs) + fs = require('fs'); + return fs; +} + +function writeOut(message) { + if (console && typeof console.error === 'function') + return console.error(message); + process._rawDebug(message); +} + +function onClose(fd) { + return function() { + lazyFs().close(fd, nop); + }; +} + +function onOpen(cb) { + return function(err, fd) { + acquiringFd = false; + if (fd !== undefined) { + cachedFd = fd; + process.on('exit', onClose(fd)); + } + cb(err, fd); + process.emit('_node_warning_fd_acquired', err, fd); + }; +} + +function onAcquired(message) { + // make a best effort attempt at writing the message + // to the fd. Errors are ignored at this point. + return function(err, fd) { + if (err) + return writeOut(message); + lazyFs().appendFile(fd, `${message}\n`, nop); + }; +} + +function acquireFd(cb) { + if (cachedFd === undefined && !acquiringFd) { + acquiringFd = true; + lazyFs().open(config.warningFile, 'a', onOpen(cb)); + } else if (cachedFd !== undefined && !acquiringFd) { + cb(null, cachedFd); + } else { + process.once('_node_warning_fd_acquired', cb); + } +} + +function output(message) { + if (typeof config.warningFile === 'string') { + acquireFd(onAcquired(message)); + return; + } + writeOut(message); +} + +function doEmitWarning(warning) { + return function() { + process.emit('warning', warning); + }; +} + function setupProcessWarnings() { if (!process.noProcessWarnings && process.env.NODE_NO_WARNINGS !== '1') { process.on('warning', (warning) => { @@ -21,7 +92,7 @@ function setupProcessWarnings() { var toString = warning.toString; if (typeof toString !== 'function') toString = Error.prototype.toString; - console.error(`${prefix}${toString.apply(warning)}`); + output(`${prefix}${toString.apply(warning)}`); } }); } @@ -44,6 +115,6 @@ function setupProcessWarnings() { if (throwDeprecation && warning.name === 'DeprecationWarning') throw warning; else - process.nextTick(() => process.emit('warning', warning)); + process.nextTick(doEmitWarning(warning)); }; } diff --git a/src/node.cc b/src/node.cc index def8e60b42c236..c5be22a7251330 100644 --- a/src/node.cc +++ b/src/node.cc @@ -199,12 +199,16 @@ bool trace_warnings = false; // that is used by lib/module.js bool config_preserve_symlinks = false; + // Set in node.cc by ParseArgs when --expose-internals or --expose_internals is // used. // Used in node_config.cc to set a constant on process.binding('config') // that is used by lib/internal/bootstrap_node.js bool config_expose_internals = false; +// Set in node.cc by ParseArgs when --redirect-warnings= is used. +const char* config_warning_file; + // process-relative uptime base, initialized at start-up static double prog_start_time; static bool debugger_running; @@ -3701,6 +3705,8 @@ static void PrintHelp() { "function is used\n" " --no-warnings silence all process warnings\n" " --trace-warnings show stack traces on process warnings\n" + " --redirect-warnings=path\n" + " write warnings to path instead of stderr\n" " --trace-sync-io show stack trace when use of sync IO\n" " is detected after the first tick\n" " --track-heap-objects track heap object allocations for heap " @@ -3765,6 +3771,7 @@ static void PrintHelp() { " prefixed to the module search path\n" "NODE_REPL_HISTORY path to the persistent REPL history file\n" "OPENSSL_CONF load OpenSSL configuration from file\n" + "NODE_REDIRECT_WARNINGS write warnings to path instead of stderr\n" "\n" "Documentation can be found at https://nodejs.org/\n"); } @@ -3866,6 +3873,8 @@ static void ParseArgs(int* argc, no_process_warnings = true; } else if (strcmp(arg, "--trace-warnings") == 0) { trace_warnings = true; + } else if (strncmp(arg, "--redirect-warnings=", 20) == 0) { + config_warning_file = arg + 20; } else if (strcmp(arg, "--trace-deprecation") == 0) { trace_deprecation = true; } else if (strcmp(arg, "--trace-sync-io") == 0) { @@ -4401,6 +4410,10 @@ void Init(int* argc, if (openssl_config.empty()) SafeGetenv("OPENSSL_CONF", &openssl_config); + if (auto redirect_warnings = secure_getenv("NODE_REDIRECT_WARNINGS")) { + config_warning_file = redirect_warnings; + } + // Parse a few arguments which are specific to Node. int v8_argc; const char** v8_argv; diff --git a/src/node_config.cc b/src/node_config.cc index 4408bc3cf33ffc..c80e3f640da99a 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -12,6 +12,7 @@ using v8::Context; using v8::Local; using v8::Object; using v8::ReadOnly; +using v8::String; using v8::Value; // The config binding is used to provide an internal view of compile or runtime @@ -47,6 +48,15 @@ void InitConfig(Local target, if (config_expose_internals) READONLY_BOOLEAN_PROPERTY("exposeInternals"); + + if (config_warning_file != nullptr) { + Local name = OneByteString(env->isolate(), "warningFile"); + Local value = String::NewFromUtf8(env->isolate(), + config_warning_file, + v8::NewStringType::kNormal) + .ToLocalChecked(); + target->DefineOwnProperty(env->context(), name, value).FromJust(); + } } // InitConfig } // namespace node diff --git a/src/node_internals.h b/src/node_internals.h index e93244a94c6927..642e61c2c26939 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -49,6 +49,11 @@ extern bool config_preserve_symlinks; // that is used by lib/internal/bootstrap_node.js extern bool config_expose_internals; +// Set in node.cc by ParseArgs when --redirect-warnings= is used. +// Used to redirect warning output to a file rather than sending +// it to stderr. +extern const char* config_warning_file; + // Forward declaration class Environment; diff --git a/test/parallel/test-process-redirect-warnings-env.js b/test/parallel/test-process-redirect-warnings-env.js new file mode 100644 index 00000000000000..86942dc9e88e11 --- /dev/null +++ b/test/parallel/test-process-redirect-warnings-env.js @@ -0,0 +1,25 @@ +'use strict'; + +// Tests the NODE_REDIRECT_WARNINGS environment variable by spawning +// a new child node process that emits a warning into a temporary +// warnings file. Once the process completes, the warning file is +// opened and the contents are validated + +const common = require('../common'); +const fs = require('fs'); +const fork = require('child_process').fork; +const path = require('path'); +const assert = require('assert'); + +common.refreshTmpDir(); + +const warnmod = require.resolve(common.fixturesDir + '/warnings.js'); +const warnpath = path.join(common.tmpDir, 'warnings.txt'); + +fork(warnmod, {env: {NODE_REDIRECT_WARNINGS: warnpath}}) + .on('exit', common.mustCall(() => { + fs.readFile(warnpath, 'utf8', common.mustCall((err, data) => { + assert.ifError(err); + assert(/\(node:\d+\) Warning: a bad practice warning/.test(data)); + })); + })); diff --git a/test/parallel/test-process-redirect-warnings.js b/test/parallel/test-process-redirect-warnings.js new file mode 100644 index 00000000000000..b798e41bf6b5e4 --- /dev/null +++ b/test/parallel/test-process-redirect-warnings.js @@ -0,0 +1,25 @@ +'use strict'; + +// Tests the --redirect-warnings command line flag by spawning +// a new child node process that emits a warning into a temporary +// warnings file. Once the process completes, the warning file is +// opened and the contents are validated + +const common = require('../common'); +const fs = require('fs'); +const fork = require('child_process').fork; +const path = require('path'); +const assert = require('assert'); + +common.refreshTmpDir(); + +const warnmod = require.resolve(common.fixturesDir + '/warnings.js'); +const warnpath = path.join(common.tmpDir, 'warnings.txt'); + +fork(warnmod, {execArgv: [`--redirect-warnings=${warnpath}`]}) + .on('exit', common.mustCall(() => { + fs.readFile(warnpath, 'utf8', common.mustCall((err, data) => { + assert.ifError(err); + assert(/\(node:\d+\) Warning: a bad practice warning/.test(data)); + })); + })); From 0f26391951b4e4623c504cc55f0a9831ce1e2352 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Tue, 10 Oct 2017 16:17:34 -0700 Subject: [PATCH 2/9] src: use SafeGetenv() for NODE_REDIRECT_WARNINGS Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. This is the part of https://github.com/nodejs/node/pull/11051 that applies to be11fb48d2f1b3f6. This part wasn't backported to 6.x when #11051 was backported because the semver-minor introduction of NODE_REDIRECT_WARNINGS hadn't been backported yet. Now that the env var is backported, this last bit of #11051 is needed. --- src/node.cc | 7 +++---- src/node_config.cc | 7 ++++--- src/node_internals.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/node.cc b/src/node.cc index c5be22a7251330..4d21f971117dbc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -207,7 +207,7 @@ bool config_preserve_symlinks = false; bool config_expose_internals = false; // Set in node.cc by ParseArgs when --redirect-warnings= is used. -const char* config_warning_file; +std::string config_warning_file; // NOLINT(runtime/string) // process-relative uptime base, initialized at start-up static double prog_start_time; @@ -4410,9 +4410,8 @@ void Init(int* argc, if (openssl_config.empty()) SafeGetenv("OPENSSL_CONF", &openssl_config); - if (auto redirect_warnings = secure_getenv("NODE_REDIRECT_WARNINGS")) { - config_warning_file = redirect_warnings; - } + if (config_warning_file.empty()) + SafeGetenv("NODE_REDIRECT_WARNINGS", &config_warning_file); // Parse a few arguments which are specific to Node. int v8_argc; diff --git a/src/node_config.cc b/src/node_config.cc index c80e3f640da99a..0e6184709642ea 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -49,11 +49,12 @@ void InitConfig(Local target, if (config_expose_internals) READONLY_BOOLEAN_PROPERTY("exposeInternals"); - if (config_warning_file != nullptr) { + if (!config_warning_file.empty()) { Local name = OneByteString(env->isolate(), "warningFile"); Local value = String::NewFromUtf8(env->isolate(), - config_warning_file, - v8::NewStringType::kNormal) + config_warning_file.data(), + v8::NewStringType::kNormal, + config_warning_file.size()) .ToLocalChecked(); target->DefineOwnProperty(env->context(), name, value).FromJust(); } diff --git a/src/node_internals.h b/src/node_internals.h index 642e61c2c26939..adcb7f835a3451 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -52,7 +52,7 @@ extern bool config_expose_internals; // Set in node.cc by ParseArgs when --redirect-warnings= is used. // Used to redirect warning output to a file rather than sending // it to stderr. -extern const char* config_warning_file; +extern std::string config_warning_file; // Forward declaration class Environment; From f83ccb4786a98013d056e3fd0f6093664c44cbdf Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 5 Apr 2017 11:45:52 -0700 Subject: [PATCH 3/9] src: use a std::vector for preload_modules A dynamically allocated array was being used, simplify the memory management by using std::vector. PR-URL: https://github.com/nodejs/node/pull/12241 Reviewed-By: Richard Lau Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen --- src/node.cc | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/src/node.cc b/src/node.cc index 4d21f971117dbc..3214d58f48fcf3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -142,8 +142,7 @@ static bool throw_deprecation = false; static bool trace_sync_io = false; static bool track_heap_objects = false; static const char* eval_string = nullptr; -static unsigned int preload_module_count = 0; -static const char** preload_modules = nullptr; +static std::vector preload_modules; #if HAVE_INSPECTOR static bool use_inspector = false; #else @@ -3328,21 +3327,18 @@ void SetupProcessObject(Environment* env, } // -r, --require - if (preload_module_count) { - CHECK(preload_modules); + if (!preload_modules.empty()) { Local array = Array::New(env->isolate()); - for (unsigned int i = 0; i < preload_module_count; ++i) { + for (unsigned int i = 0; i < preload_modules.size(); ++i) { Local module = String::NewFromUtf8(env->isolate(), - preload_modules[i]); + preload_modules[i].c_str()); array->Set(i, module); } READONLY_PROPERTY(process, "_preload_modules", array); - delete[] preload_modules; - preload_modules = nullptr; - preload_module_count = 0; + preload_modules.clear(); } // --no-deprecation @@ -3798,13 +3794,11 @@ static void ParseArgs(int* argc, const char** new_exec_argv = new const char*[nargs]; const char** new_v8_argv = new const char*[nargs]; const char** new_argv = new const char*[nargs]; - const char** local_preload_modules = new const char*[nargs]; for (unsigned int i = 0; i < nargs; ++i) { new_exec_argv[i] = nullptr; new_v8_argv[i] = nullptr; new_argv[i] = nullptr; - local_preload_modules[i] = nullptr; } // exec_argv starts with the first option, the other two start with argv[0]. @@ -3862,7 +3856,7 @@ static void ParseArgs(int* argc, exit(9); } args_consumed += 1; - local_preload_modules[preload_module_count++] = module; + preload_modules.push_back(module); } else if (strcmp(arg, "--check") == 0 || strcmp(arg, "-c") == 0) { syntax_check_only = true; } else if (strcmp(arg, "--interactive") == 0 || strcmp(arg, "-i") == 0) { @@ -3952,16 +3946,6 @@ static void ParseArgs(int* argc, memcpy(argv, new_argv, new_argc * sizeof(*argv)); delete[] new_argv; *argc = static_cast(new_argc); - - // Copy the preload_modules from the local array to an appropriately sized - // global array. - if (preload_module_count > 0) { - CHECK(!preload_modules); - preload_modules = new const char*[preload_module_count]; - memcpy(preload_modules, local_preload_modules, - preload_module_count * sizeof(*preload_modules)); - } - delete[] local_preload_modules; } From 6b47fdcd65f24c84b4dc7a6f4a33b8e25f7b2bba Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 20 Feb 2017 06:18:43 -0800 Subject: [PATCH 4/9] src: allow CLI args in env with NODE_OPTIONS Not all CLI options are supported, those that are problematic from a security or implementation point of view are disallowed, as are ones that are inappropriate (for example, -e, -p, --i), or that only make sense when changed with code changes (such as options that change the javascript syntax or add new APIs). PR-URL: https://github.com/nodejs/node/pull/12028 Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Refael Ackermann Reviewed-By: Gibson Fahnestock Reviewed-By: Bradley Farias --- configure | 8 ++ doc/api/cli.md | 35 +++++ doc/node.1 | 7 + src/node.cc | 169 +++++++++++++++++++------ test/parallel/test-cli-node-options.js | 74 +++++++++++ vcbuild.bat | 6 +- 6 files changed, 260 insertions(+), 39 deletions(-) create mode 100644 test/parallel/test-cli-node-options.js diff --git a/configure b/configure index 7272e19261b238..1eaf26c0955575 100755 --- a/configure +++ b/configure @@ -444,6 +444,11 @@ parser.add_option('--without-ssl', dest='without_ssl', help='build without SSL (disables crypto, https, inspector, etc.)') +parser.add_option('--without-node-options', + action='store_true', + dest='without_node_options', + help='build without NODE_OPTIONS support') + parser.add_option('--xcode', action='store_true', dest='use_xcode', @@ -975,6 +980,9 @@ def configure_openssl(o): o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0 if options.use_openssl_ca_store: o['defines'] += ['NODE_OPENSSL_CERT_STORE'] + o['variables']['node_without_node_options'] = b(options.without_node_options) + if options.without_node_options: + o['defines'] += ['NODE_WITHOUT_NODE_OPTIONS'] if options.openssl_fips: o['variables']['openssl_fips'] = options.openssl_fips fips_dir = os.path.join(root_dir, 'deps', 'openssl', 'fips') diff --git a/doc/api/cli.md b/doc/api/cli.md index 09fabb77334ff3..8346a262d9c433 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -332,6 +332,41 @@ added: v6.11.0 When set to `1`, process warnings are silenced. +### `NODE_OPTIONS=options...` + + +`options...` are interpreted as if they had been specified on the command line +before the actual command line (so they can be overriden). Node will exit with +an error if an option that is not allowed in the environment is used, such as +`-p` or a script file. + +Node options that are allowed are: +- `--enable-fips` +- `--force-fips` +- `--icu-data-dir` +- `--no-deprecation` +- `--no-warnings` +- `--openssl-config` +- `--prof-process` +- `--redirect-warnings` +- `--require`, `-r` +- `--throw-deprecation` +- `--trace-deprecation` +- `--trace-events-enabled` +- `--trace-sync-io` +- `--trace-warnings` +- `--track-heap-objects` +- `--use-bundled-ca` +- `--use-openssl-ca` +- `--v8-pool-size` +- `--zero-fill-buffers` + +V8 options that are allowed are: +- `--max_old_space_size` + + ### `NODE_REPL_HISTORY=file`