From 6e255adb423d8fc4938aa150d8567648eb979ec5 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Sun, 5 Mar 2017 03:05:40 -0500 Subject: [PATCH 1/7] cli: ensure --check flag works for piped-in code Previously, the --check CLI flag had no effect when run on code piped from stdin. This commit updates the bootstrap logic to handle the --check flag the same way regardless of whether the code is piped from stdin. Fixes: https://github.com/nodejs/node/issues/11680 --- lib/internal/bootstrap_node.js | 30 ++++++++++++++++++++---------- test/parallel/test-cli-syntax.js | 13 +++++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index aea83fc331e771..c37c4dafd7e34f 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -132,18 +132,11 @@ // check if user passed `-c` or `--check` arguments to Node. if (process._syntax_check_only != null) { - const vm = NativeModule.require('vm'); const fs = NativeModule.require('fs'); - const internalModule = NativeModule.require('internal/module'); // read the source const filename = Module._resolveFilename(process.argv[1]); var source = fs.readFileSync(filename, 'utf-8'); - // remove shebang and BOM - source = internalModule.stripBOM(source.replace(/^#!.*/, '')); - // wrap it - source = Module.wrap(source); - // compile the script, this will throw if it fails - new vm.Script(source, {filename: filename, displayErrors: true}); + checkScriptSyntax(source); process.exit(0); } @@ -184,8 +177,12 @@ }); process.stdin.on('end', function() { - process._eval = code; - evalScript('[stdin]'); + if (process._syntax_check_only != null) { + checkScriptSyntax(code); + } else { + process._eval = code; + evalScript('[stdin]'); + } }); } } @@ -445,6 +442,19 @@ } } + function checkScriptSyntax(source) { + const Module = NativeModule.require('module'); + const vm = NativeModule.require('vm'); + const internalModule = NativeModule.require('internal/module'); + + // remove shebang and BOM + source = internalModule.stripBOM(source.replace(/^#!.*/, '')); + // wrap it + source = Module.wrap(source); + // compile the script, this will throw if it fails + new vm.Script(source, {displayErrors: true}); + } + // Below you find a minimal module system, which is used to load the node // core modules found in lib/*.js. All core modules are compiled into the // node binary, so they can be loaded faster. diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index d7781eddffbb42..c438d7dab53343 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -82,3 +82,16 @@ const syntaxArgs = [ assert.strictEqual(c.status, 1, 'code == ' + c.status); }); }); + +// should not execute code piped from stdin with --check +// loop each possible option, `-c` or `--check` +syntaxArgs.forEach(function(args) { + const stdin = 'throw new Error("should not get run");'; + const c = spawnSync(node, args, {encoding: 'utf8', input: stdin}); + + // no stdout or stderr should be produced + assert.strictEqual(c.stdout, '', 'stdout produced'); + assert.strictEqual(c.stderr, '', 'stderr produced'); + + assert.strictEqual(c.status, 0, 'code == ' + c.status); +}); From bc820d5636c452634817abeb5000d303d0e67b3c Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Sun, 5 Mar 2017 21:01:09 -0500 Subject: [PATCH 2/7] squash: ensure the filename is still used --- lib/internal/bootstrap_node.js | 8 ++++---- test/parallel/test-cli-syntax.js | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index c37c4dafd7e34f..b4ed16573e1e07 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -136,7 +136,7 @@ // read the source const filename = Module._resolveFilename(process.argv[1]); var source = fs.readFileSync(filename, 'utf-8'); - checkScriptSyntax(source); + checkScriptSyntax(source, filename); process.exit(0); } @@ -178,7 +178,7 @@ process.stdin.on('end', function() { if (process._syntax_check_only != null) { - checkScriptSyntax(code); + checkScriptSyntax(code, '[stdin]'); } else { process._eval = code; evalScript('[stdin]'); @@ -442,7 +442,7 @@ } } - function checkScriptSyntax(source) { + function checkScriptSyntax(source, filename) { const Module = NativeModule.require('module'); const vm = NativeModule.require('vm'); const internalModule = NativeModule.require('internal/module'); @@ -452,7 +452,7 @@ // wrap it source = Module.wrap(source); // compile the script, this will throw if it fails - new vm.Script(source, {displayErrors: true}); + new vm.Script(source, {displayErrors: true, filename}); } // Below you find a minimal module system, which is used to load the node diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index c438d7dab53343..432ad3ce628da4 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -52,6 +52,9 @@ const syntaxArgs = [ // no stdout should be produced assert.strictEqual(c.stdout, '', 'stdout produced'); + // stderr should include the filename + assert(c.stderr.startsWith(file), "stderr doesn't start with the filename"); + // stderr should have a syntax error message const match = c.stderr.match(/^SyntaxError: Unexpected identifier$/m); assert(match, 'stderr incorrect'); @@ -95,3 +98,22 @@ syntaxArgs.forEach(function(args) { assert.strictEqual(c.status, 0, 'code == ' + c.status); }); + +// should should throw if code piped from stdin with --check has bad syntax +// loop each possible option, `-c` or `--check` +syntaxArgs.forEach(function(args) { + const stdin = 'var foo bar;'; + const c = spawnSync(node, args, {encoding: 'utf8', input: stdin}); + + // stderr should include '[stdin]' as the filename + assert(c.stderr.startsWith('[stdin]'), "stderr doesn't start with [stdin]"); + + // no stdout or stderr should be produced + assert.strictEqual(c.stdout, '', 'stdout produced'); + + // stderr should have a syntax error message + const match = c.stderr.match(/^SyntaxError: Unexpected identifier$/m); + assert(match, 'stderr incorrect'); + + assert.strictEqual(c.status, 1, 'code == ' + c.status); +}); From 24ad2bf27d491be07cda645e7326c3e7a51e202b Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Thu, 30 Mar 2017 02:35:03 -0400 Subject: [PATCH 3/7] cli: throw when -c and -e are used simultaneously The -c flag ("check script syntax") and -e flag ("evaluate given code") have contradictory meanings. Make them mutually exclusive by throwing when both of them are provided. --- lib/internal/bootstrap_node.js | 4 ++++ test/parallel/test-cli-syntax.js | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index b4ed16573e1e07..ba6f90e9e1170f 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -114,6 +114,10 @@ } if (process._eval != null && !process._forceRepl) { + if (process._syntax_check_only != null) { + console.error('--check and --eval flags are mutually exclusive.'); + process.exit(1); + } // User passed '-e' or '--eval' arguments to Node without '-i' or // '--interactive' preloadModules(); diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index 432ad3ce628da4..b4481920ba43cf 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -117,3 +117,18 @@ syntaxArgs.forEach(function(args) { assert.strictEqual(c.status, 1, 'code == ' + c.status); }); + +// should throw if -c and -e flags are both passed +['-c', '--check'].forEach(function(checkFlag) { + ['-e', '--eval'].forEach(function(evalFlag) { + const args = [checkFlag, evalFlag, 'foo']; + const c = spawnSync(node, args, {encoding: 'utf8'}); + + assert.strictEqual( + c.stderr, + '--check and --eval flags are mutually exclusive.\n' + ); + + assert.strictEqual(c.status, 1, 'code == ' + c.status); + }); +}); From 913d121e606d2af73cc9e84fb166d8ae32bd68e6 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Thu, 30 Mar 2017 14:43:41 -0400 Subject: [PATCH 4/7] squash: change exit code to 9 for invalid args --- lib/internal/bootstrap_node.js | 2 +- test/parallel/test-cli-syntax.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index ba6f90e9e1170f..82fd0f909473e0 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -116,7 +116,7 @@ if (process._eval != null && !process._forceRepl) { if (process._syntax_check_only != null) { console.error('--check and --eval flags are mutually exclusive.'); - process.exit(1); + process.exit(9); } // User passed '-e' or '--eval' arguments to Node without '-i' or // '--interactive' diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index b4481920ba43cf..d8659d596047c6 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -129,6 +129,6 @@ syntaxArgs.forEach(function(args) { '--check and --eval flags are mutually exclusive.\n' ); - assert.strictEqual(c.status, 1, 'code == ' + c.status); + assert.strictEqual(c.status, 9, 'code == ' + c.status); }); }); From 55787017960dd93879bd78065c1dc0a3dac31f45 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Thu, 30 Mar 2017 14:45:14 -0400 Subject: [PATCH 5/7] squash: remove duplicate word in commit --- test/parallel/test-cli-syntax.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index d8659d596047c6..854aa2ed7f74a5 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -99,7 +99,7 @@ syntaxArgs.forEach(function(args) { assert.strictEqual(c.status, 0, 'code == ' + c.status); }); -// should should throw if code piped from stdin with --check has bad syntax +// should throw if code piped from stdin with --check has bad syntax // loop each possible option, `-c` or `--check` syntaxArgs.forEach(function(args) { const stdin = 'var foo bar;'; From 299155d554a51185b9870ec64c1348461b0ea41e Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Sat, 1 Apr 2017 03:20:07 -0400 Subject: [PATCH 6/7] squash: make assertion error message more specific --- test/parallel/test-cli-syntax.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index 854aa2ed7f74a5..d0baca3aec583a 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -129,6 +129,6 @@ syntaxArgs.forEach(function(args) { '--check and --eval flags are mutually exclusive.\n' ); - assert.strictEqual(c.status, 9, 'code == ' + c.status); + assert.strictEqual(c.status, 9, 'code === ' + c.status); }); }); From fab87a95585dfd4277642bc3883b59a59695813e Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Sat, 1 Apr 2017 03:19:40 -0400 Subject: [PATCH 7/7] squash: move flag validation to c++ --- lib/internal/bootstrap_node.js | 4 ---- src/node.cc | 6 ++++++ test/parallel/test-cli-syntax.js | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 82fd0f909473e0..b4ed16573e1e07 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -114,10 +114,6 @@ } if (process._eval != null && !process._forceRepl) { - if (process._syntax_check_only != null) { - console.error('--check and --eval flags are mutually exclusive.'); - process.exit(9); - } // User passed '-e' or '--eval' arguments to Node without '-i' or // '--interactive' preloadModules(); diff --git a/src/node.cc b/src/node.cc index 77e6a5826ee957..2b5b6152c3b949 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3800,6 +3800,12 @@ static void ParseArgs(int* argc, } #endif + if (eval_string != nullptr && syntax_check_only) { + fprintf(stderr, + "%s: either --check or --eval can be used, not both\n", argv[0]); + exit(9); + } + // Copy remaining arguments. const unsigned int args_left = nargs - index; memcpy(new_argv + new_argc, argv + index, args_left * sizeof(*argv)); diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js index d0baca3aec583a..2275ce40a50d85 100644 --- a/test/parallel/test-cli-syntax.js +++ b/test/parallel/test-cli-syntax.js @@ -126,7 +126,7 @@ syntaxArgs.forEach(function(args) { assert.strictEqual( c.stderr, - '--check and --eval flags are mutually exclusive.\n' + `${node}: either --check or --eval can be used, not both\n` ); assert.strictEqual(c.status, 9, 'code === ' + c.status);