Skip to content

Commit cf0ccb9

Browse files
committed
src: fix positional args in task runner
1 parent c5cfdd4 commit cf0ccb9

File tree

5 files changed

+57
-37
lines changed

5 files changed

+57
-37
lines changed

src/node_task_runner.cc

+40-28
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
@@ -128,7 +138,7 @@ ProcessRunner::ProcessRunner(
128138
// EscapeShell escapes a string to be used as a command line argument.
129139
// It replaces single quotes with "\\'" and double quotes with "\\\"".
130140
// It also removes excessive quote pairs and handles edge cases.
131-
std::string EscapeShell(const std::string& input) {
141+
std::string EscapeShell(const std::string_view input) {
132142
// If the input is an empty string, return a pair of quotes
133143
if (input.empty()) {
134144
return "''";
@@ -140,11 +150,12 @@ std::string EscapeShell(const std::string& input) {
140150
// Check if input contains any forbidden characters
141151
// If it doesn't, return the input as is.
142152
if (input.find_first_of(forbidden_characters) == std::string::npos) {
143-
return input;
153+
return std::string(input);
144154
}
145155

146156
// Replace single quotes("'") with "\\'"
147-
std::string escaped = std::regex_replace(input, std::regex("'"), "\\'");
157+
std::string escaped =
158+
std::regex_replace(std::string(input), std::regex("'"), "\\'");
148159

149160
// Wrap the result in single quotes
150161
escaped = "'" + escaped + "'";
@@ -188,7 +199,7 @@ void ProcessRunner::Run() {
188199

189200
void RunTask(std::shared_ptr<InitializationResultImpl> result,
190201
std::string_view command_id,
191-
const std::optional<std::string>& positional_args) {
202+
const std::vector<std::string_view>& positional_args) {
192203
std::string_view path = "package.json";
193204
std::string raw_json;
194205

@@ -256,20 +267,21 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
256267
// If the "--" flag is not found, it returns an empty optional.
257268
// Otherwise, it returns the positional arguments as a single string.
258269
// Example: "node -- script.js arg1 arg2" returns "arg1 arg2".
259-
std::optional<std::string> GetPositionalArgs(
260-
const std::vector<std::string>& args) {
270+
PositionalArgs GetPositionalArgs(const std::vector<std::string>& args) {
261271
// If the "--" flag is not found, return an empty optional
262272
// Otherwise, return the positional arguments as a single string
263273
if (auto dash_dash = std::find(args.begin(), args.end(), "--");
264274
dash_dash != args.end()) {
265-
std::string positional_args;
275+
PositionalArgs positional_args{};
266276
for (auto it = dash_dash + 1; it != args.end(); ++it) {
267-
positional_args += it->c_str();
277+
// SAFETY: The following code is safe because the lifetime of the arguments
278+
// is guaranteed to be valid until the end of the task runner.
279+
positional_args.push_back(std::string_view(it->c_str(), it->size()));
268280
}
269281
return positional_args;
270282
}
271283

272-
return std::nullopt;
284+
return {};
273285
}
274286

275287
} // namespace node::task_runner

src/node_task_runner.h

+6-5
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/fixtures/run-script/node_modules/.bin/positional-args

+2-1
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

+6-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

test/parallel/test-node-run.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,11 @@ 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+
assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/);
64+
assert.match(child.stdout, /The total number of arguments are: 4/);
6465
assert.strictEqual(child.stderr, '');
6566
assert.strictEqual(child.code, 0);
6667
});

0 commit comments

Comments
 (0)