FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix positional args in task runner · nodejs/node@d169d0f · GitHub

/ node Public

Commit d169d0f

Browse files
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

6 files changed

Lines changed: 109 additions & 44 deletions

File tree

‎src/node_task_runner.cc‎

Lines 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

‎src/node_task_runner.h‎

Lines 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

‎test/cctest/test_node_task_runner.cc‎

Lines 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);

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

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 15 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines 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)
Back | FazBrowse Home | New Git URL