Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d169d0f

Browse filesBrowse files
anonrigtargos
anonrig
authored and
targos
committed
src: fix positional args in task runner
PR-URL: #52810 Fixes: #52740 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 5055663 commit d169d0f
Copy full SHA for d169d0f

File tree

Expand file treeCollapse file tree

6 files changed

+109
-44
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+109
-44
lines changed
Open diff view settings
Collapse file

‎src/node_task_runner.cc‎

Copy file name to clipboardExpand all lines: src/node_task_runner.cc
+62-33Lines changed: 62 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@
66
namespace node::task_runner {
77

88
#ifdef _WIN32
9-
static constexpr char bin_path[] = "\\node_modules\\.bin";
9+
static constexpr const char* bin_path = "\\node_modules\\.bin";
1010
#else
11-
static constexpr char bin_path[] = "/node_modules/.bin";
11+
static constexpr const char* bin_path = "/node_modules/.bin";
1212
#endif // _WIN32
1313

14-
ProcessRunner::ProcessRunner(
15-
std::shared_ptr<InitializationResultImpl> result,
16-
std::string_view command,
17-
const std::optional<std::string>& positional_args) {
14+
ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
15+
std::string_view command,
16+
const PositionalArgs& positional_args) {
1817
memset(&options_, 0, sizeof(uv_process_options_t));
1918

2019
// Get the current working directory.
@@ -54,10 +53,6 @@ ProcessRunner::ProcessRunner(
5453

5554
std::string command_str(command);
5655

57-
if (positional_args.has_value()) {
58-
command_str += " " + EscapeShell(positional_args.value());
59-
}
60-
6156
// Set environment variables
6257
uv_env_item_t* env_items;
6358
int env_count;
@@ -69,33 +64,45 @@ ProcessRunner::ProcessRunner(
6964
// ProcessRunner instance.
7065
for (int i = 0; i < env_count; i++) {
7166
std::string name = env_items[i].name;
72-
std::string value = env_items[i].value;
67+
auto value = env_items[i].value;
7368

7469
#ifdef _WIN32
7570
// We use comspec environment variable to find cmd.exe path on Windows
7671
// Example: 'C:\\Windows\\system32\\cmd.exe'
7772
// If we don't find it, we fallback to 'cmd.exe' for Windows
78-
if (name.size() == 7 && StringEqualNoCaseN(name.c_str(), "comspec", 7)) {
73+
if (StringEqualNoCase(name.c_str(), "comspec")) {
7974
file_ = value;
8075
}
8176
#endif // _WIN32
8277

8378
// Check if environment variable key is matching case-insensitive "path"
84-
if (name.size() == 4 && StringEqualNoCaseN(name.c_str(), "path", 4)) {
85-
value.insert(0, current_bin_path);
79+
if (StringEqualNoCase(name.c_str(), "path")) {
80+
env_vars_.push_back(name + "=" + current_bin_path + value);
81+
} else {
82+
// Environment variables should be in "KEY=value" format
83+
env_vars_.push_back(name + "=" + value);
8684
}
87-
88-
// Environment variables should be in "KEY=value" format
89-
value.insert(0, name + "=");
90-
env_vars_.push_back(value);
9185
}
9286
uv_os_free_environ(env_items, env_count);
9387

9488
// Use the stored reference on the instance.
9589
options_.file = file_.c_str();
9690

91+
// Add positional arguments to the command string.
92+
// Note that each argument needs to be escaped.
93+
if (!positional_args.empty()) {
94+
for (const auto& arg : positional_args) {
95+
command_str += " " + EscapeShell(arg);
96+
}
97+
}
98+
9799
#ifdef _WIN32
98-
if (file_.find("cmd.exe") != std::string::npos) {
100+
// We check whether file_ ends with cmd.exe in a case-insensitive manner.
101+
// C++20 provides ends_with, but we roll our own for compatibility.
102+
const char* cmdexe = "cmd.exe";
103+
if (file_.size() >= strlen(cmdexe) &&
104+
StringEqualNoCase(cmdexe,
105+
file_.c_str() + file_.size() - strlen(cmdexe))) {
99106
// If the file is cmd.exe, use the following command line arguments:
100107
// "/c" Carries out the command and exit.
101108
// "/d" Disables execution of AutoRun commands.
@@ -104,6 +111,9 @@ ProcessRunner::ProcessRunner(
104111
command_args_ = {
105112
options_.file, "/d", "/s", "/c", "\"" + command_str + "\""};
106113
} else {
114+
// If the file is not cmd.exe, and it is unclear wich shell is being used,
115+
// so assume -c is the correct syntax (Unix-like shells use -c for this
116+
// purpose).
107117
command_args_ = {options_.file, "-c", command_str};
108118
}
109119
#else
@@ -126,12 +136,19 @@ ProcessRunner::ProcessRunner(
126136
}
127137

128138
// EscapeShell escapes a string to be used as a command line argument.
139+
// Under Windows, we follow:
140+
// https://daviddeley.com/autohotkey/parameters/parameters.htm
141+
// Elsewhere:
129142
// It replaces single quotes with "\\'" and double quotes with "\\\"".
130143
// It also removes excessive quote pairs and handles edge cases.
131-
std::string EscapeShell(const std::string& input) {
144+
std::string EscapeShell(const std::string_view input) {
132145
// If the input is an empty string, return a pair of quotes
133146
if (input.empty()) {
147+
#ifdef _WIN32
148+
return "\"\"";
149+
#else
134150
return "''";
151+
#endif
135152
}
136153

137154
static const std::string_view forbidden_characters =
@@ -140,21 +157,32 @@ std::string EscapeShell(const std::string& input) {
140157
// Check if input contains any forbidden characters
141158
// If it doesn't, return the input as is.
142159
if (input.find_first_of(forbidden_characters) == std::string::npos) {
143-
return input;
160+
return std::string(input);
144161
}
145162

146-
// Replace single quotes("'") with "\\'"
147-
std::string escaped = std::regex_replace(input, std::regex("'"), "\\'");
163+
static const std::regex leadingQuotePairs("^(?:'')+(?!$)");
148164

149-
// Wrap the result in single quotes
165+
#ifdef _WIN32
166+
// Replace double quotes with single quotes and surround the string
167+
// with double quotes for Windows.
168+
std::string escaped =
169+
std::regex_replace(std::string(input), std::regex("\""), "\"\"");
170+
escaped = "\"" + escaped + "\"";
171+
// Remove excessive quote pairs and handle edge cases
172+
static const std::regex tripleSingleQuote("\\\\\"\"\"");
173+
escaped = std::regex_replace(escaped, leadingQuotePairs, "");
174+
escaped = std::regex_replace(escaped, tripleSingleQuote, "\\\"");
175+
#else
176+
// Replace single quotes("'") with "\\'" and wrap the result
177+
// in single quotes.
178+
std::string escaped =
179+
std::regex_replace(std::string(input), std::regex("'"), "\\'");
150180
escaped = "'" + escaped + "'";
151-
152181
// Remove excessive quote pairs and handle edge cases
153-
static const std::regex leadingQuotePairs("^(?:'')+(?!$)");
154182
static const std::regex tripleSingleQuote("\\\\'''");
155-
156183
escaped = std::regex_replace(escaped, leadingQuotePairs, "");
157184
escaped = std::regex_replace(escaped, tripleSingleQuote, "\\'");
185+
#endif // _WIN32
158186

159187
return escaped;
160188
}
@@ -188,7 +216,7 @@ void ProcessRunner::Run() {
188216

189217
void RunTask(std::shared_ptr<InitializationResultImpl> result,
190218
std::string_view command_id,
191-
const std::optional<std::string>& positional_args) {
219+
const std::vector<std::string_view>& positional_args) {
192220
std::string_view path = "package.json";
193221
std::string raw_json;
194222

@@ -256,20 +284,21 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
256284
// If the "--" flag is not found, it returns an empty optional.
257285
// Otherwise, it returns the positional arguments as a single string.
258286
// Example: "node -- script.js arg1 arg2" returns "arg1 arg2".
259-
std::optional<std::string> GetPositionalArgs(
260-
const std::vector<std::string>& args) {
287+
PositionalArgs GetPositionalArgs(const std::vector<std::string>& args) {
261288
// If the "--" flag is not found, return an empty optional
262289
// Otherwise, return the positional arguments as a single string
263290
if (auto dash_dash = std::find(args.begin(), args.end(), "--");
264291
dash_dash != args.end()) {
265-
std::string positional_args;
292+
PositionalArgs positional_args{};
266293
for (auto it = dash_dash + 1; it != args.end(); ++it) {
267-
positional_args += it->c_str();
294+
// SAFETY: The following code is safe because the lifetime of the
295+
// arguments is guaranteed to be valid until the end of the task runner.
296+
positional_args.push_back(std::string_view(it->c_str(), it->size()));
268297
}
269298
return positional_args;
270299
}
271300

272-
return std::nullopt;
301+
return {};
273302
}
274303

275304
} // namespace node::task_runner
Collapse file

‎src/node_task_runner.h‎

Copy file name to clipboardExpand all lines: src/node_task_runner.h
+6-5Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
namespace node {
1515
namespace task_runner {
1616

17+
using PositionalArgs = std::vector<std::string_view>;
18+
1719
// ProcessRunner is the class responsible for running a process.
1820
// A class instance is created for each process to be run.
1921
// The class is responsible for spawning the process and handling its exit.
@@ -22,7 +24,7 @@ class ProcessRunner {
2224
public:
2325
ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
2426
std::string_view command_id,
25-
const std::optional<std::string>& positional_args);
27+
const PositionalArgs& positional_args);
2628
void Run();
2729
static void ExitCallback(uv_process_t* req,
2830
int64_t exit_status,
@@ -51,10 +53,9 @@ class ProcessRunner {
5153

5254
void RunTask(std::shared_ptr<InitializationResultImpl> result,
5355
std::string_view command_id,
54-
const std::optional<std::string>& positional_args);
55-
std::optional<std::string> GetPositionalArgs(
56-
const std::vector<std::string>& args);
57-
std::string EscapeShell(const std::string& command);
56+
const PositionalArgs& positional_args);
57+
PositionalArgs GetPositionalArgs(const std::vector<std::string>& args);
58+
std::string EscapeShell(const std::string_view command);
5859

5960
} // namespace task_runner
6061
} // namespace node
Collapse file

‎test/cctest/test_node_task_runner.cc‎

Copy file name to clipboardExpand all lines: test/cctest/test_node_task_runner.cc
+17-1Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,20 @@ class TaskRunnerTest : public EnvironmentTestFixture {};
99

1010
TEST_F(TaskRunnerTest, EscapeShell) {
1111
std::vector<std::pair<std::string, std::string>> expectations = {
12+
#ifdef _WIN32
13+
{"", "\"\""},
14+
{"test", "test"},
15+
{"test words", "\"test words\""},
16+
{"$1", "\"$1\""},
17+
{"\"$1\"", "\"\"\"$1\"\"\""},
18+
{"'$1'", "\"'$1'\""},
19+
{"\\$1", "\"\\$1\""},
20+
{"--arg=\"$1\"", "\"--arg=\"\"$1\"\"\""},
21+
{"--arg=node exec -c \"$1\"", "\"--arg=node exec -c \"\"$1\"\"\""},
22+
{"--arg=node exec -c '$1'", "\"--arg=node exec -c '$1'\""},
23+
{"'--arg=node exec -c \"$1\"'", "\"'--arg=node exec -c \"\"$1\"\"'\""}
24+
25+
#else
1226
{"", "''"},
1327
{"test", "test"},
1428
{"test words", "'test words'"},
@@ -19,7 +33,9 @@ TEST_F(TaskRunnerTest, EscapeShell) {
1933
{"--arg=\"$1\"", "'--arg=\"$1\"'"},
2034
{"--arg=node exec -c \"$1\"", "'--arg=node exec -c \"$1\"'"},
2135
{"--arg=node exec -c '$1'", "'--arg=node exec -c \\'$1\\''"},
22-
{"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"}};
36+
{"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"}
37+
#endif
38+
};
2339

2440
for (const auto& [input, expected] : expectations) {
2541
EXPECT_EQ(node::task_runner::EscapeShell(input), expected);
Collapse file

‎test/fixtures/run-script/node_modules/.bin/positional-args‎

Copy file name to clipboardExpand all lines: test/fixtures/run-script/node_modules/.bin/positional-args
+2-1Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/run-script/node_modules/.bin/positional-args.bat‎

Copy file name to clipboardExpand all lines: test/fixtures/run-script/node_modules/.bin/positional-args.bat
+15-2Lines changed: 15 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/parallel/test-node-run.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-node-run.js
+7-2Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,15 @@ describe('node run [command]', () => {
5757
it('appends positional arguments', async () => {
5858
const child = await common.spawnPromisified(
5959
process.execPath,
60-
[ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"'],
60+
[ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"', 'A', 'B', 'C'],
6161
{ cwd: fixtures.path('run-script') },
6262
);
63-
assert.match(child.stdout, /--help "hello world test"/);
63+
if (common.isWindows) {
64+
assert.match(child.stdout, /Arguments: '--help ""hello world test"" A B C'/);
65+
} else {
66+
assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/);
67+
}
68+
assert.match(child.stdout, /The total number of arguments are: 4/);
6469
assert.strictEqual(child.stderr, '');
6570
assert.strictEqual(child.code, 0);
6671
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.