From 9db146943894ec22fb9c62f5146b23d54e3a5d45 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 2 May 2024 17:07:11 -0400 Subject: [PATCH 1/7] src: fix positional args in task runner --- src/node_task_runner.cc | 68 +++++++++++-------- src/node_task_runner.h | 11 +-- .../node_modules/.bin/positional-args | 3 +- .../node_modules/.bin/positional-args.bat | 7 +- test/parallel/test-node-run.js | 5 +- 5 files changed, 57 insertions(+), 37 deletions(-) diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index a719314d245718..237cd2dae9fc4f 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -6,15 +6,14 @@ namespace node::task_runner { #ifdef _WIN32 -static constexpr char bin_path[] = "\\node_modules\\.bin"; +static constexpr const char* bin_path = "\\node_modules\\.bin"; #else -static constexpr char bin_path[] = "/node_modules/.bin"; +static constexpr const char* bin_path = "/node_modules/.bin"; #endif // _WIN32 -ProcessRunner::ProcessRunner( - std::shared_ptr result, - std::string_view command, - const std::optional& positional_args) { +ProcessRunner::ProcessRunner(std::shared_ptr result, + std::string_view command, + const PositionalArgs& positional_args) { memset(&options_, 0, sizeof(uv_process_options_t)); // Get the current working directory. @@ -54,10 +53,6 @@ ProcessRunner::ProcessRunner( std::string command_str(command); - if (positional_args.has_value()) { - command_str += " " + EscapeShell(positional_args.value()); - } - // Set environment variables uv_env_item_t* env_items; int env_count; @@ -69,33 +64,45 @@ ProcessRunner::ProcessRunner( // ProcessRunner instance. for (int i = 0; i < env_count; i++) { std::string name = env_items[i].name; - std::string value = env_items[i].value; + auto value = env_items[i].value; #ifdef _WIN32 // We use comspec environment variable to find cmd.exe path on Windows // Example: 'C:\\Windows\\system32\\cmd.exe' // If we don't find it, we fallback to 'cmd.exe' for Windows - if (name.size() == 7 && StringEqualNoCaseN(name.c_str(), "comspec", 7)) { + if (StringEqualNoCase(name.c_str(), "comspec")) { file_ = value; } #endif // _WIN32 // Check if environment variable key is matching case-insensitive "path" - if (name.size() == 4 && StringEqualNoCaseN(name.c_str(), "path", 4)) { - value.insert(0, current_bin_path); + if (StringEqualNoCase(name.c_str(), "path")) { + env_vars_.push_back(name + "=" + current_bin_path + value); + } else { + // Environment variables should be in "KEY=value" format + env_vars_.push_back(name + "=" + value); } - - // Environment variables should be in "KEY=value" format - value.insert(0, name + "="); - env_vars_.push_back(value); } uv_os_free_environ(env_items, env_count); // Use the stored reference on the instance. options_.file = file_.c_str(); + // Add positional arguments to the command string. + // Note that each argument needs to be escaped. + if (!positional_args.empty()) { + for (const auto& arg : positional_args) { + command_str += " " + EscapeShell(arg); + } + } + #ifdef _WIN32 - if (file_.find("cmd.exe") != std::string::npos) { + // We check whether file_ ends with cmd.exe in a case-insensitive manner. + // C++20 provides ends_with, but we roll our own for compatibility. + const char* cmdexe = "cmd.exe"; + if (file_.size() >= strlen(cmdexe) && + StringEqualNoCase(cmdexe, + file_.c_str() + file_.size() - strlen(cmdexe))) { // If the file is cmd.exe, use the following command line arguments: // "/c" Carries out the command and exit. // "/d" Disables execution of AutoRun commands. @@ -104,6 +111,9 @@ ProcessRunner::ProcessRunner( command_args_ = { options_.file, "/d", "/s", "/c", "\"" + command_str + "\""}; } else { + // If the file is not cmd.exe, and it is unclear wich shell is being used, + // so assume -c is the correct syntax (Unix-like shells use -c for this + // purpose). command_args_ = {options_.file, "-c", command_str}; } #else @@ -128,7 +138,7 @@ ProcessRunner::ProcessRunner( // EscapeShell escapes a string to be used as a command line argument. // It replaces single quotes with "\\'" and double quotes with "\\\"". // It also removes excessive quote pairs and handles edge cases. -std::string EscapeShell(const std::string& input) { +std::string EscapeShell(const std::string_view input) { // If the input is an empty string, return a pair of quotes if (input.empty()) { return "''"; @@ -140,11 +150,12 @@ std::string EscapeShell(const std::string& input) { // Check if input contains any forbidden characters // If it doesn't, return the input as is. if (input.find_first_of(forbidden_characters) == std::string::npos) { - return input; + return std::string(input); } // Replace single quotes("'") with "\\'" - std::string escaped = std::regex_replace(input, std::regex("'"), "\\'"); + std::string escaped = + std::regex_replace(std::string(input), std::regex("'"), "\\'"); // Wrap the result in single quotes escaped = "'" + escaped + "'"; @@ -188,7 +199,7 @@ void ProcessRunner::Run() { void RunTask(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args) { + const std::vector& positional_args) { std::string_view path = "package.json"; std::string raw_json; @@ -256,20 +267,21 @@ void RunTask(std::shared_ptr result, // If the "--" flag is not found, it returns an empty optional. // Otherwise, it returns the positional arguments as a single string. // Example: "node -- script.js arg1 arg2" returns "arg1 arg2". -std::optional GetPositionalArgs( - const std::vector& args) { +PositionalArgs GetPositionalArgs(const std::vector& args) { // If the "--" flag is not found, return an empty optional // Otherwise, return the positional arguments as a single string if (auto dash_dash = std::find(args.begin(), args.end(), "--"); dash_dash != args.end()) { - std::string positional_args; + PositionalArgs positional_args{}; for (auto it = dash_dash + 1; it != args.end(); ++it) { - positional_args += it->c_str(); + // SAFETY: The following code is safe because the lifetime of the + // arguments is guaranteed to be valid until the end of the task runner. + positional_args.push_back(std::string_view(it->c_str(), it->size())); } return positional_args; } - return std::nullopt; + return {}; } } // namespace node::task_runner diff --git a/src/node_task_runner.h b/src/node_task_runner.h index 0d5bea9bcb7d29..3d8973db98ab42 100644 --- a/src/node_task_runner.h +++ b/src/node_task_runner.h @@ -14,6 +14,8 @@ namespace node { namespace task_runner { +using PositionalArgs = std::vector; + // ProcessRunner is the class responsible for running a process. // A class instance is created for each process to be run. // The class is responsible for spawning the process and handling its exit. @@ -22,7 +24,7 @@ class ProcessRunner { public: ProcessRunner(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args); + const PositionalArgs& positional_args); void Run(); static void ExitCallback(uv_process_t* req, int64_t exit_status, @@ -51,10 +53,9 @@ class ProcessRunner { void RunTask(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args); -std::optional GetPositionalArgs( - const std::vector& args); -std::string EscapeShell(const std::string& command); + const PositionalArgs& positional_args); +PositionalArgs GetPositionalArgs(const std::vector& args); +std::string EscapeShell(const std::string_view command); } // namespace task_runner } // namespace node diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args b/test/fixtures/run-script/node_modules/.bin/positional-args index 47d2d7d6a26d93..2d8092378ba1da 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args +++ b/test/fixtures/run-script/node_modules/.bin/positional-args @@ -1,2 +1,3 @@ #!/bin/bash -echo $@ +echo "Arguments: '$@'" +echo "The total number of arguments are: $#" diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args.bat b/test/fixtures/run-script/node_modules/.bin/positional-args.bat index b95ea66f90851a..6e62ab7783b127 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args.bat +++ b/test/fixtures/run-script/node_modules/.bin/positional-args.bat @@ -1,2 +1,7 @@ +@echo off @shift -@echo %* +set argv=0 +for %%x in (%*) do Set /A argv+=1 + +@echo Arguments: '%*' +@echo The total number of arguments are: %argv% diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js index 69a7c9d9a46db1..c3b639da0e15f8 100644 --- a/test/parallel/test-node-run.js +++ b/test/parallel/test-node-run.js @@ -57,10 +57,11 @@ describe('node run [command]', () => { it('appends positional arguments', async () => { const child = await common.spawnPromisified( process.execPath, - [ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"'], + [ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"', 'A', 'B', 'C'], { cwd: fixtures.path('run-script') }, ); - assert.match(child.stdout, /--help "hello world test"/); + assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/); + assert.match(child.stdout, /The total number of arguments are: 4/); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); From 62564bb1370bced42c58d0ba0ffed79d7a4b1d2d Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 3 May 2024 16:39:47 -0400 Subject: [PATCH 2/7] fix: improve windows support --- src/node_task_runner.cc | 9 ++++++++- .../run-script/node_modules/.bin/positional-args.bat | 11 ++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 237cd2dae9fc4f..5602b7296b7d30 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -153,12 +153,19 @@ std::string EscapeShell(const std::string_view input) { return std::string(input); } +#ifdef _WIN32 + // Wrap the result in double quotes + // Passing "--help "sdfsd"" to a batch script + // will be considered as a single argument made + // of --help "sdfsd". However, there are corner cases. + std::string escaped = "\"" + escaped + "\""; +#else // Replace single quotes("'") with "\\'" std::string escaped = std::regex_replace(std::string(input), std::regex("'"), "\\'"); - // Wrap the result in single quotes escaped = "'" + escaped + "'"; +#endif // Remove excessive quote pairs and handle edge cases static const std::regex leadingQuotePairs("^(?:'')+(?!$)"); diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args.bat b/test/fixtures/run-script/node_modules/.bin/positional-args.bat index 6e62ab7783b127..650ba4e182433d 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args.bat +++ b/test/fixtures/run-script/node_modules/.bin/positional-args.bat @@ -1,7 +1,12 @@ @echo off @shift +setlocal enabledelayedexpansion set argv=0 -for %%x in (%*) do Set /A argv+=1 - -@echo Arguments: '%*' +set "output=" +for %%x in (%*) do ( + Set /A argv+=1 + Set "arg=%%~x" + Set "output=!output! %%~x" +) +@echo Arguments: %output% @echo The total number of arguments are: %argv% From 66e07212e29e3a255a957e9168d03e6d6cb662d2 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 3 May 2024 17:12:50 -0400 Subject: [PATCH 3/7] fix: update tests --- src/node_task_runner.cc | 7 +++++++ test/cctest/test_node_task_runner.cc | 18 +++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 5602b7296b7d30..256e37754b1baf 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -136,12 +136,19 @@ ProcessRunner::ProcessRunner(std::shared_ptr result, } // EscapeShell escapes a string to be used as a command line argument. +// Under Windows, we follow: +// https://daviddeley.com/autohotkey/parameters/parameters.htm +// Elsewhere: // It replaces single quotes with "\\'" and double quotes with "\\\"". // It also removes excessive quote pairs and handles edge cases. std::string EscapeShell(const std::string_view input) { // If the input is an empty string, return a pair of quotes if (input.empty()) { +#ifdef _WIN32 + return "\"\""; +#else return "''"; +#endif } static const std::string_view forbidden_characters = diff --git a/test/cctest/test_node_task_runner.cc b/test/cctest/test_node_task_runner.cc index 5e30b979f3d1d7..48479bfa475bd0 100644 --- a/test/cctest/test_node_task_runner.cc +++ b/test/cctest/test_node_task_runner.cc @@ -9,6 +9,20 @@ class TaskRunnerTest : public EnvironmentTestFixture {}; TEST_F(TaskRunnerTest, EscapeShell) { std::vector> expectations = { +#ifdef _WIN32 + {"", "\"\""}, + {"test", "test"}, + {"test words", "\"test words\""}, + {"$1", "\"$1\""}, + {"\"$1\"", "\"\"$1\"\""}, + {"'$1'", "\"'$1'\""}, + {"\\$1", "\"\\$1\""}, + {"--arg=\"$1\"", "\"--arg=\"$1\"\""}, + {"--arg=node exec -c \"$1\"", "\"--arg=node exec -c \"$1\"\""}, + {"--arg=node exec -c '$1'", "\"--arg=node exec -c '$1'\""}, + {"'--arg=node exec -c \"$1\"'", "\"'--arg=node exec -c \"$1\"'\""} + +#else {"", "''"}, {"test", "test"}, {"test words", "'test words'"}, @@ -19,7 +33,9 @@ TEST_F(TaskRunnerTest, EscapeShell) { {"--arg=\"$1\"", "'--arg=\"$1\"'"}, {"--arg=node exec -c \"$1\"", "'--arg=node exec -c \"$1\"'"}, {"--arg=node exec -c '$1'", "'--arg=node exec -c \\'$1\\''"}, - {"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"}}; + {"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"} +#endif +}; for (const auto& [input, expected] : expectations) { EXPECT_EQ(node::task_runner::EscapeShell(input), expected); From 9a44f9ecb25893365b1d4fe4beea3f1a95249cda Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 3 May 2024 18:24:08 -0400 Subject: [PATCH 4/7] lint --- test/cctest/test_node_task_runner.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cctest/test_node_task_runner.cc b/test/cctest/test_node_task_runner.cc index 48479bfa475bd0..6dec339f5d0a5f 100644 --- a/test/cctest/test_node_task_runner.cc +++ b/test/cctest/test_node_task_runner.cc @@ -35,7 +35,7 @@ TEST_F(TaskRunnerTest, EscapeShell) { {"--arg=node exec -c '$1'", "'--arg=node exec -c \\'$1\\''"}, {"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"} #endif -}; + }; for (const auto& [input, expected] : expectations) { EXPECT_EQ(node::task_runner::EscapeShell(input), expected); From 4989987dc649593e2f5764238f08c32bd37a23ad Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 3 May 2024 20:46:10 -0400 Subject: [PATCH 5/7] use correct value --- src/node_task_runner.cc | 2 +- .../run-script/node_modules/.bin/positional-args.bat | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 256e37754b1baf..b20c0486c6c59e 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -165,7 +165,7 @@ std::string EscapeShell(const std::string_view input) { // Passing "--help "sdfsd"" to a batch script // will be considered as a single argument made // of --help "sdfsd". However, there are corner cases. - std::string escaped = "\"" + escaped + "\""; + std::string escaped = "\"" + std::string(input) + "\""; #else // Replace single quotes("'") with "\\'" std::string escaped = diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args.bat b/test/fixtures/run-script/node_modules/.bin/positional-args.bat index 650ba4e182433d..39339d49c47995 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args.bat +++ b/test/fixtures/run-script/node_modules/.bin/positional-args.bat @@ -1,12 +1,14 @@ @echo off -@shift setlocal enabledelayedexpansion set argv=0 set "output=" for %%x in (%*) do ( Set /A argv+=1 - Set "arg=%%~x" - Set "output=!output! %%~x" + if "!output!" == "" ( + Set "output=%%~x" + ) else ( + Set "output=!output! %%~x" + ) ) -@echo Arguments: %output% +@echo Arguments: '%output%' @echo The total number of arguments are: %argv% From 75059026a2063a0e39ab6e9f62cea99f9647d7f5 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 5 May 2024 11:59:34 -0400 Subject: [PATCH 6/7] simplify windows tests --- src/node_task_runner.cc | 25 +++++++++++-------- .../node_modules/.bin/positional-args.bat | 1 + test/parallel/test-node-run.js | 6 ++++- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index b20c0486c6c59e..4b7d541340a79d 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -160,26 +160,29 @@ std::string EscapeShell(const std::string_view input) { return std::string(input); } + static const std::regex leadingQuotePairs("^(?:'')+(?!$)"); + #ifdef _WIN32 - // Wrap the result in double quotes - // Passing "--help "sdfsd"" to a batch script - // will be considered as a single argument made - // of --help "sdfsd". However, there are corner cases. - std::string escaped = "\"" + std::string(input) + "\""; + // Replace double quotes with single quotes and surround the string + // with double quotes for Windows. + std::string escaped = + std::regex_replace(std::string(input), std::regex("\""), "\"\""); + escaped = "\"" + escaped + "\""; + // Remove excessive quote pairs and handle edge cases + static const std::regex tripleSingleQuote("\\\\\"\"\""); + escaped = std::regex_replace(escaped, leadingQuotePairs, ""); + escaped = std::regex_replace(escaped, tripleSingleQuote, "\\\""); #else - // Replace single quotes("'") with "\\'" + // Replace single quotes("'") with "\\'" and wrap the result + // in single quotes. std::string escaped = std::regex_replace(std::string(input), std::regex("'"), "\\'"); - // Wrap the result in single quotes escaped = "'" + escaped + "'"; -#endif - // Remove excessive quote pairs and handle edge cases - static const std::regex leadingQuotePairs("^(?:'')+(?!$)"); static const std::regex tripleSingleQuote("\\\\'''"); - escaped = std::regex_replace(escaped, leadingQuotePairs, ""); escaped = std::regex_replace(escaped, tripleSingleQuote, "\\'"); +#endif // _WIN32 return escaped; } diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args.bat b/test/fixtures/run-script/node_modules/.bin/positional-args.bat index 39339d49c47995..4b94ed34d51daf 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args.bat +++ b/test/fixtures/run-script/node_modules/.bin/positional-args.bat @@ -10,5 +10,6 @@ for %%x in (%*) do ( Set "output=!output! %%~x" ) ) +@echo Raw '%*' @echo Arguments: '%output%' @echo The total number of arguments are: %argv% diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js index c3b639da0e15f8..11a5b413148f4e 100644 --- a/test/parallel/test-node-run.js +++ b/test/parallel/test-node-run.js @@ -60,7 +60,11 @@ describe('node run [command]', () => { [ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"', 'A', 'B', 'C'], { cwd: fixtures.path('run-script') }, ); - assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/); + if (common.isWindows) { + assert.match(child.stdout, /Arguments: '--help ""hello world test"" A B C'/); + } else { + assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/); + } assert.match(child.stdout, /The total number of arguments are: 4/); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); From f388eec26d9620278c82f2d8b6834cf2710f37ba Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 6 May 2024 18:05:04 -0400 Subject: [PATCH 7/7] fix cctests --- test/cctest/test_node_task_runner.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/cctest/test_node_task_runner.cc b/test/cctest/test_node_task_runner.cc index 6dec339f5d0a5f..a2c520d2709f2e 100644 --- a/test/cctest/test_node_task_runner.cc +++ b/test/cctest/test_node_task_runner.cc @@ -14,13 +14,13 @@ TEST_F(TaskRunnerTest, EscapeShell) { {"test", "test"}, {"test words", "\"test words\""}, {"$1", "\"$1\""}, - {"\"$1\"", "\"\"$1\"\""}, + {"\"$1\"", "\"\"\"$1\"\"\""}, {"'$1'", "\"'$1'\""}, {"\\$1", "\"\\$1\""}, - {"--arg=\"$1\"", "\"--arg=\"$1\"\""}, - {"--arg=node exec -c \"$1\"", "\"--arg=node exec -c \"$1\"\""}, + {"--arg=\"$1\"", "\"--arg=\"\"$1\"\"\""}, + {"--arg=node exec -c \"$1\"", "\"--arg=node exec -c \"\"$1\"\"\""}, {"--arg=node exec -c '$1'", "\"--arg=node exec -c '$1'\""}, - {"'--arg=node exec -c \"$1\"'", "\"'--arg=node exec -c \"$1\"'\""} + {"'--arg=node exec -c \"$1\"'", "\"'--arg=node exec -c \"\"$1\"\"'\""} #else {"", "''"},