Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: allow CLI args in env with NODE_OPTIONS #12028

Merged
merged 1 commit into from
Apr 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,11 @@ parser.add_option('--without-ssl',
dest='without_ssl',
help='build without SSL')

parser.add_option('--without-node-options',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until I finish #12425 we'll need this in vcbuild.bat, sorry for the hassle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want this implemented for Windows? If so, you'll have to give me a hint as to how. Also, I'm not sure it has to be added. Lots of other configure options aren't implemented in vcbuild.bat, I'm not sure this one is so useful it has to exist for Windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't have an opinion about this specific option. But I don't the disparity to widen.

vcbuild.bat | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vcbuild.bat b/vcbuild.bat
index 86f2991887..f168167fcc 100644
--- a/vcbuild.bat
+++ b/vcbuild.bat
@@ -90,6 +90,7 @@ if /i "%1"=="download-all"  set download_arg="--download=all"&goto arg-ok
 if /i "%1"=="ignore-flaky"  set test_args=%test_args% --flaky-tests=dontcare&goto arg-ok
 if /i "%1"=="enable-vtune"  set enable_vtune_arg=1&goto arg-ok
 if /i "%1"=="dll"           set dll=1&goto arg-ok
+if /i "%1"=="no-NODE-OPTIONS"	set no_NODE_OPTIONS=1&goto arg-ok
 
 echo Error: invalid command line option `%1`.
 exit /b 1
@@ -121,6 +122,7 @@ if defined release_urlbase set configure_flags=%configure_flags% --release-urlba
 if defined download_arg set configure_flags=%configure_flags% %download_arg%
 if defined enable_vtune_arg set configure_flags=%configure_flags% --enable-vtune-profiling
 if defined dll set configure_flags=%configure_flags% --shared
+if defined no_NODE_OPTIONS set configure_flags=%configure_flags% --without-node-options
 
 if "%i18n_arg%"=="full-icu" set configure_flags=%configure_flags% --with-intl=full-icu
 if "%i18n_arg%"=="small-icu" set configure_flags=%configure_flags% --with-intl=small-icu
@@ -439,7 +441,7 @@ echo Failed to create vc project files.
 goto exit
 
 :help
-echo vcbuild.bat [debug/release] [msi] [test-all/test-uv/test-inspector/test-internet/test-pummel/test-simple/test-message] [clean] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vc2015] [download-all] [enable-vtune] [lint/lint-ci]
+echo vcbuild.bat [debug/release] [msi] [test-all/test-uv/test-inspector/test-internet/test-pummel/test-simple/test-message] [clean] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vc2015] [download-all] [enable-vtune] [lint/lint-ci] [no-NODE-OPTIONS]
 echo Examples:
 echo   vcbuild.bat                : builds release build
 echo   vcbuild.bat debug          : builds debug build

action='store_true',
dest='without_node_options',
help='build without NODE_OPTIONS support')

parser.add_option('--xcode',
action='store_true',
dest='use_xcode',
Expand Down Expand Up @@ -965,6 +970,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')
Expand Down
34 changes: 34 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,40 @@ added: v7.5.0

When set to `1`, process warnings are silenced.

### `NODE_OPTIONS=options...`
<!-- YAML
added: REPLACEME
-->

`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_PENDING_DEPRECATION=1`
<!-- YAML
added: REPLACEME
Expand Down
7 changes: 7 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,13 @@ with small\-icu support.
.BR NODE_NO_WARNINGS =\fI1\fR
When set to \fI1\fR, process warnings are silenced.

.TP
.BR NODE_OPTIONS =\fIoptions...\fR
\fBoptions...\fR 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 \fB-p\fR or a script file.

.TP
.BR NODE_PATH =\fIpath\fR[:\fI...\fR]
\':\'\-separated list of directories prefixed to the module search path.
Expand Down
170 changes: 133 additions & 37 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3626,6 +3626,9 @@ static void PrintHelp() {
#endif
#endif
"NODE_NO_WARNINGS set to 1 to silence process warnings\n"
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
"NODE_OPTIONS set CLI options in the environment\n"
#endif
#ifdef _WIN32
"NODE_PATH ';'-separated list of directories\n"
#else
Expand All @@ -3642,6 +3645,51 @@ static void PrintHelp() {
}


static void CheckIfAllowedInEnv(const char* exe, bool is_env,
const char* arg) {
if (!is_env)
return;

// Find the arg prefix when its --some_arg=val
const char* eq = strchr(arg, '=');
size_t arglen = eq ? eq - arg : strlen(arg);

static const char* whitelist[] = {
// Node options
"-r", "--require",
"--no-deprecation",
"--no-warnings",
"--trace-warnings",
"--redirect-warnings",
"--trace-deprecation",
"--trace-sync-io",
"--trace-events-enabled",
"--track-heap-objects",
"--throw-deprecation",
"--zero-fill-buffers",
"--v8-pool-size",
"--use-openssl-ca",
"--use-bundled-ca",
"--enable-fips",
"--force-fips",
"--openssl-config",
"--icu-data-dir",

// V8 options
"--max_old_space_size",
};

for (unsigned i = 0; i < arraysize(whitelist); i++) {
const char* allowed = whitelist[i];
if (strlen(allowed) == arglen && strncmp(allowed, arg, arglen) == 0)
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the simplicity of this, it's not the most performant approach. Would like to see this optimized more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions?

Note:

  • The conditional is copied verbatim from the existing code, it is how all command line options are currently matched.
  • It would be more efficient to inline the checks into the actual options parsing so the comparison would be done once, but I was requested to refactor that blacklisting approach into a whitelisting approach so no one could accidentally add an option and forget to blacklist it.
  • None of this code runs unless NODE_OPTIONS is actually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what to do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of options I can think of include:

  1. using a hash table, problem might be that cost of building the hashtable outweighs the benefits
  2. have 26 whitelist arrays (or 2 dimention array) so that we can quickly cut down the list by the first letter in the option and then only search the subset for that list. This would reduce the search time, while being 0 cost in terms of startup time. The main drawback is that is does require us to make sure things go into the right array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the code ends up being a bit more complicated but the amount of time it takes to go through the various options is much shorter.


fprintf(stderr, "%s: %s is not allowed in NODE_OPTIONS\n", exe, arg);
exit(9);
}


// Parse command line arguments.
//
// argv is modified in place. exec_argv and v8_argv are out arguments that
Expand All @@ -3658,7 +3706,8 @@ static void ParseArgs(int* argc,
int* exec_argc,
const char*** exec_argv,
int* v8_argc,
const char*** v8_argv) {
const char*** v8_argv,
bool is_env) {
const unsigned int nargs = static_cast<unsigned int>(*argc);
const char** new_exec_argv = new const char*[nargs];
const char** new_v8_argv = new const char*[nargs];
Expand Down Expand Up @@ -3687,6 +3736,8 @@ static void ParseArgs(int* argc,
const char* const arg = argv[index];
unsigned int args_consumed = 1;

CheckIfAllowedInEnv(argv[0], is_env, arg);

if (debug_options.ParseOption(arg)) {
// Done, consumed by DebugOptions::ParseOption().
} else if (strcmp(arg, "--version") == 0 || strcmp(arg, "-v") == 0) {
Expand Down Expand Up @@ -3839,6 +3890,13 @@ static void ParseArgs(int* argc,

// Copy remaining arguments.
const unsigned int args_left = nargs - index;

if (is_env && args_left) {
fprintf(stderr, "%s: %s is not supported in NODE_OPTIONS\n",
argv[0], argv[index]);
exit(9);
}

memcpy(new_argv + new_argc, argv + index, args_left * sizeof(*argv));
new_argc += args_left;

Expand Down Expand Up @@ -4161,6 +4219,54 @@ inline void PlatformInit() {
}


void ProcessArgv(int* argc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just makes me happy...

const char** argv,
int* exec_argc,
const char*** exec_argv,
bool is_env = false) {
// Parse a few arguments which are specific to Node.
int v8_argc;
const char** v8_argv;
ParseArgs(argc, argv, exec_argc, exec_argv, &v8_argc, &v8_argv, is_env);

// TODO(bnoordhuis) Intercept --prof arguments and start the CPU profiler
// manually? That would give us a little more control over its runtime
// behavior but it could also interfere with the user's intentions in ways
// we fail to anticipate. Dillema.
for (int i = 1; i < v8_argc; ++i) {
if (strncmp(v8_argv[i], "--prof", sizeof("--prof") - 1) == 0) {
v8_is_profiling = true;
break;
}
}

#ifdef __POSIX__
// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
// performance penalty of frequent EINTR wakeups when the profiler is running.
// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
if (v8_is_profiling) {
uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
}
#endif

// The const_cast doesn't violate conceptual const-ness. V8 doesn't modify
// the argv array or the elements it points to.
if (v8_argc > 1)
V8::SetFlagsFromCommandLine(&v8_argc, const_cast<char**>(v8_argv), true);

// Anything that's still in v8_argv is not a V8 or a node option.
for (int i = 1; i < v8_argc; i++) {
fprintf(stderr, "%s: bad option: %s\n", argv[0], v8_argv[i]);
}
delete[] v8_argv;
v8_argv = nullptr;

if (v8_argc > 1) {
exit(9);
}
}


void Init(int* argc,
const char** argv,
int* exec_argc,
Expand Down Expand Up @@ -4214,31 +4320,36 @@ void Init(int* argc,
SafeGetenv("OPENSSL_CONF", &openssl_config);
#endif

// Parse a few arguments which are specific to Node.
int v8_argc;
const char** v8_argv;
ParseArgs(argc, argv, exec_argc, exec_argv, &v8_argc, &v8_argv);

// TODO(bnoordhuis) Intercept --prof arguments and start the CPU profiler
// manually? That would give us a little more control over its runtime
// behavior but it could also interfere with the user's intentions in ways
// we fail to anticipate. Dillema.
for (int i = 1; i < v8_argc; ++i) {
if (strncmp(v8_argv[i], "--prof", sizeof("--prof") - 1) == 0) {
v8_is_profiling = true;
break;
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
std::string node_options;
if (SafeGetenv("NODE_OPTIONS", &node_options)) {
// Smallest tokens are 2-chars (a not space and a space), plus 2 extra
// pointers, for the prepended executable name, and appended NULL pointer.
size_t max_len = 2 + (node_options.length() + 1) / 2;
const char** argv_from_env = new const char*[max_len];
int argc_from_env = 0;
// [0] is expected to be the program name, fill it in from the real argv.
argv_from_env[argc_from_env++] = argv[0];

char* cstr = strdup(node_options.c_str());
char* initptr = cstr;
char* token;
while ((token = strtok(initptr, " "))) { // NOLINT(runtime/threadsafe_fn)
initptr = nullptr;
argv_from_env[argc_from_env++] = token;
}
}

#ifdef __POSIX__
// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
// performance penalty of frequent EINTR wakeups when the profiler is running.
// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
if (v8_is_profiling) {
uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
argv_from_env[argc_from_env] = nullptr;
int exec_argc_;
const char** exec_argv_ = nullptr;
ProcessArgv(&argc_from_env, argv_from_env, &exec_argc_, &exec_argv_, true);
delete[] exec_argv_;
delete[] argv_from_env;
free(cstr);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd rather that C++ std lib classes are used to avoid all the manual allocations. And you shouldn't have to worry about if the C++ std lib is available on a certain platform. And you don't need to rely on the non-reentrant legacy c strtok.

  std::vector<std::string> env_args;
  std::string::size_type pos = 0, pos_next = 0;
  while (pos_next < nodeopt.size())
  {
    pos_next = nodeopt.find(" ", pos);
    if (pos_next == std::string::npos)
    {
      pos_next = nodeopt.size();
    }

    if (pos_next > pos)
    {
      env_args.push_back(nodeopt.substr(pos, pos_next - pos));
    }

    pos = pos_next + 1;
  }

  std::vector<const char *> argv_from_env;
  argv_from_env.reserve(args.size() + 2);
  argv_from_env.push_back(argv[0]);
  for (size_t i = 0; i < args.size(); i++)
  {
    argv_from_env.push_back(env_args[i].c_str());
  }
  argv_from_env.push_back(nullptr);

and now you can call ProcessArgv:

    ProcessArgv(&argc_from_env, &argv_from_env[0], &exec_argc_, &exec_argv_, true );

}
#endif

ProcessArgv(argc, argv, exec_argc, exec_argv);

#if defined(NODE_HAVE_I18N_SUPPORT)
// If the parameter isn't given, use the env variable.
if (icu_data_dir.empty())
Expand All @@ -4250,21 +4361,6 @@ void Init(int* argc,
"(check NODE_ICU_DATA or --icu-data-dir parameters)");
}
#endif
// The const_cast doesn't violate conceptual const-ness. V8 doesn't modify
// the argv array or the elements it points to.
if (v8_argc > 1)
V8::SetFlagsFromCommandLine(&v8_argc, const_cast<char**>(v8_argv), true);

// Anything that's still in v8_argv is not a V8 or a node option.
for (int i = 1; i < v8_argc; i++) {
fprintf(stderr, "%s: bad option: %s\n", argv[0], v8_argv[i]);
}
delete[] v8_argv;
v8_argv = nullptr;

if (v8_argc > 1) {
exit(9);
}

// Unconditionally force typed arrays to allocate outside the v8 heap. This
// is to prevent memory pointers from being moved around that are returned by
Expand Down
76 changes: 76 additions & 0 deletions test/parallel/test-cli-node-options.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
'use strict';
const common = require('../common');
if (process.config.variables.node_without_node_options)
return common.skip('missing NODE_OPTIONS support');

// Test options specified by env variable.

const assert = require('assert');
const exec = require('child_process').execFile;

disallow('--version');
disallow('-v');
disallow('--help');
disallow('-h');
disallow('--eval');
disallow('-e');
disallow('--print');
disallow('-p');
disallow('-pe');
disallow('--check');
disallow('-c');
disallow('--interactive');
disallow('-i');
disallow('--v8-options');
disallow('--');

function disallow(opt) {
const options = {env: {NODE_OPTIONS: opt}};
exec(process.execPath, options, common.mustCall(function(err) {
const message = err.message.split(/\r?\n/)[1];
const expect = process.execPath + ': ' + opt +
' is not allowed in NODE_OPTIONS';

assert.strictEqual(err.code, 9);
assert.strictEqual(message, expect);
}));
}

const printA = require.resolve('../fixtures/printA.js');

expect('-r ' + printA, 'A\nB\n');
expect('--no-deprecation', 'B\n');
expect('--no-warnings', 'B\n');
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');
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('--use-openssl-ca', 'B\n');
expect('--use-bundled-ca', 'B\n');
expect('--openssl-config=_ossl_cfg', 'B\n');
expect('--icu-data-dir=_d', 'B\n');

// V8 options
expect('--max_old_space_size=0', 'B\n');

function expect(opt, want) {
const printB = require.resolve('../fixtures/printB.js');
const argv = [printB];
const opts = {
env: {NODE_OPTIONS: opt},
maxBuffer: 1000000000,
};
exec(process.execPath, argv, opts, common.mustCall(function(err, stdout) {
assert.ifError(err);
if (!RegExp(want).test(stdout)) {
console.error('For %j, failed to find %j in: <\n%s\n>',
opt, expect, stdout);
assert(false, 'Expected ' + expect);
}
}));
}
Loading