Skip to content

Commit d41b8e9

Browse files
Erica VellanowethCopilot
Erica Vellanoweth
and
Copilot
committed
Fix ExecuteCommand && chaining regression
The refactor in 4b55e79 that moved ExecuteCommand from Dependencies to Core/Components replaced GetCommandsToExecute() (split on &&, loop) with GetCommandToExecute() (single command), completely removing && splitting. This broke all profiles using chained commands (PERF-REDIS, PERF-MEMCACHED, GET-STARTED-REDIS, PERF-NETWORK). Changes: - Restore && split-and-loop in ExecuteAsync for UseShell=false (default) - UseShell=true path preserved: wraps in bash -c / cmd /C as single command - Quote-aware SplitCommands method skips && inside single/double quotes - Restore sudo auto-propagation for chained commands - Add tests for chaining, sudo propagation, profile-matching commands, relative paths with working directory, and Windows chaining Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 7953f8d commit d41b8e9

2 files changed

Lines changed: 244 additions & 43 deletions

File tree

‎src/VirtualClient/VirtualClient.Core.UnitTests/Components/ExecuteCommandTests.cs‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,149 @@ public async Task ExecuteCommandSupportsCommandChainingOnUnixSystems(string full
106106
}
107107
}
108108

109+
[Test]
110+
[TestCase("/home/user/anycommand&&/home/user/anyothercommand", "/home/user/anycommand;/home/user/anyothercommand")]
111+
[TestCase("/home/user/anycommand --argument=value&&/home/user/anyothercommand --argument2=value2", "/home/user/anycommand --argument=value;/home/user/anyothercommand --argument2=value2")]
112+
[TestCase("sudo anycommand&&anyothercommand", "sudo anycommand;sudo anyothercommand")]
113+
[TestCase("sudo /home/user/anycommand&&/home/user/anyothercommand", "sudo /home/user/anycommand;sudo /home/user/anyothercommand")]
114+
[TestCase("sudo /home/user/anycommand --argument=value&&/home/user/anyothercommand --argument2=value2", "sudo /home/user/anycommand --argument=value;sudo /home/user/anyothercommand --argument2=value2")]
115+
public async Task ExecuteCommandSplitsAndExecutesChainedCommandsSequentiallyOnUnixSystems(string fullCommand, string expectedCommandExecuted)
116+
{
117+
this.SetupDefaults(PlatformID.Unix);
118+
119+
using (TestExecuteCommand command = new TestExecuteCommand(this.mockFixture))
120+
{
121+
command.Parameters[nameof(ExecuteCommand.Command)] = fullCommand;
122+
List<string> expectedCommands = new List<string>(expectedCommandExecuted.Split(';'));
123+
124+
this.mockFixture.ProcessManager.OnProcessCreated = (process) =>
125+
{
126+
expectedCommands.Remove(process.FullCommand());
127+
};
128+
129+
await command.ExecuteAsync(CancellationToken.None);
130+
Assert.IsEmpty(expectedCommands);
131+
}
132+
}
133+
134+
[Test]
135+
[TestCase(
136+
"sudo dmesg && sudo lsblk && sudo mount && sudo df -h && sudo find /sys -name scheduler -print",
137+
"sudo dmesg;sudo lsblk;sudo mount;sudo df -h;sudo find /sys -name scheduler -print")]
138+
public async Task ExecuteCommandSplitsChainedCommandsWithWhitespace(string fullCommand, string expectedCommandExecuted)
139+
{
140+
// Bug Scenario:
141+
// Spaces (whitespace) in the commands due to the chaining SHOULD NOT cause
142+
// parsing issues.
143+
144+
this.SetupDefaults(PlatformID.Unix);
145+
146+
using (TestExecuteCommand command = new TestExecuteCommand(this.mockFixture))
147+
{
148+
command.Parameters[nameof(ExecuteCommand.Command)] = fullCommand;
149+
List<string> expectedCommands = new List<string>(expectedCommandExecuted.Split(';'));
150+
151+
this.mockFixture.ProcessManager.OnProcessCreated = (process) =>
152+
{
153+
expectedCommands.Remove(process.FullCommand());
154+
};
155+
156+
await command.ExecuteAsync(CancellationToken.None);
157+
Assert.IsEmpty(expectedCommands);
158+
}
159+
}
160+
161+
[Test]
162+
[TestCase(
163+
"git checkout 1.4.0&&autoreconf -ivf&&bash -c './configure'&&make",
164+
"git checkout 1.4.0;autoreconf -ivf;bash -c './configure';make")]
165+
public async Task ExecuteCommandSplitsChainedCommandsMatchingProfileUsage(string fullCommand, string expectedCommandExecuted)
166+
{
167+
// Scenario: Real profile usage from PERF-REDIS, PERF-MEMCACHED, GET-STARTED-REDIS.
168+
// Commands chained with && should be split and executed as separate processes.
169+
170+
this.SetupDefaults(PlatformID.Unix);
171+
172+
using (TestExecuteCommand command = new TestExecuteCommand(this.mockFixture))
173+
{
174+
command.Parameters[nameof(ExecuteCommand.Command)] = fullCommand;
175+
List<string> expectedCommands = new List<string>(expectedCommandExecuted.Split(';'));
176+
177+
this.mockFixture.ProcessManager.OnProcessCreated = (process) =>
178+
{
179+
expectedCommands.Remove(process.FullCommand());
180+
};
181+
182+
await command.ExecuteAsync(CancellationToken.None);
183+
Assert.IsEmpty(expectedCommands);
184+
}
185+
}
186+
187+
[Test]
188+
[TestCase(
189+
"dos2unix install-packages.sh&&dos2unix install-python.sh&&dos2unix install-pwsh.sh&&dos2unix install-docker.sh",
190+
"dos2unix install-packages.sh;dos2unix install-python.sh;dos2unix install-pwsh.sh;dos2unix install-docker.sh")]
191+
public async Task ExecuteCommandSplitsChainedCommandsWithRelativePaths(string fullCommand, string expectedCommandExecuted)
192+
{
193+
// Bug Scenario:
194+
// Relative paths used to reference scripts in a specific working directory using a globally installed
195+
// Linux toolset (e.g. dos2unix) should be left as-is. However, when a working directory is defined, that directory should
196+
// be added to the PATH environment variable.
197+
198+
this.SetupDefaults(PlatformID.Unix, Architecture.Arm64);
199+
200+
this.mockFixture.Parameters[nameof(ExecuteCommand.WorkingDirectory)] = "{PackagePath/Platform:system_setup}";
201+
202+
this.mockFixture.PackageManager.OnGetPackage("system_setup")
203+
.ReturnsAsync(new DependencyPath("system_setup", "/microsoft-labs/VirtualClient/content/linux-arm64/packages/system_setup.1.0.0"));
204+
205+
string expectedWorkingDirectory = "/microsoft-labs/VirtualClient/content/linux-arm64/packages/system_setup.1.0.0/linux-arm64";
206+
207+
using (TestExecuteCommand command = new TestExecuteCommand(this.mockFixture))
208+
{
209+
command.Parameters[nameof(ExecuteCommand.Command)] = fullCommand;
210+
List<string> expectedCommands = new List<string>(expectedCommandExecuted.Split(';'));
211+
212+
this.mockFixture.ProcessManager.OnProcessCreated = (process) =>
213+
{
214+
expectedCommands.Remove(process.FullCommand());
215+
216+
// Expect:
217+
// The working directory of the process should be set.
218+
Assert.AreEqual(expectedWorkingDirectory, process.StartInfo.WorkingDirectory);
219+
220+
// Expect:
221+
// The working directory should be added to the PATH environment variable.
222+
Assert.IsTrue(this.mockFixture.PlatformSpecifics.EnvironmentVariables[EnvironmentVariable.PATH].Contains(expectedWorkingDirectory));
223+
};
224+
225+
await command.ExecuteAsync(CancellationToken.None);
226+
Assert.IsEmpty(expectedCommands);
227+
}
228+
}
229+
230+
[Test]
231+
[TestCase("C:\\\\Users\\User\\anycommand&&C:\\\\home\\user\\anyothercommand", "C:\\\\Users\\User\\anycommand;C:\\\\home\\user\\anyothercommand")]
232+
[TestCase("C:\\\\Users\\User\\anycommand --argument=1&&C:\\\\home\\user\\anyothercommand --argument=2", "C:\\\\Users\\User\\anycommand --argument=1;C:\\\\home\\user\\anyothercommand --argument=2")]
233+
public async Task ExecuteCommandSplitsAndExecutesChainedCommandsSequentiallyOnWindowsSystems(string fullCommand, string expectedCommandExecuted)
234+
{
235+
this.SetupDefaults(PlatformID.Win32NT);
236+
237+
using (TestExecuteCommand command = new TestExecuteCommand(this.mockFixture))
238+
{
239+
command.Parameters[nameof(ExecuteCommand.Command)] = fullCommand;
240+
List<string> expectedCommands = new List<string>(expectedCommandExecuted.Split(';'));
241+
242+
this.mockFixture.ProcessManager.OnProcessCreated = (process) =>
243+
{
244+
expectedCommands.Remove(process.FullCommand());
245+
};
246+
247+
await command.ExecuteAsync(CancellationToken.None);
248+
Assert.IsEmpty(expectedCommands);
249+
}
250+
}
251+
109252
[Test]
110253
[TestCase("/home/user/anycommand&&/home/user/anyothercommand", "bash -c \"/home/user/anycommand&&/home/user/anyothercommand\"")]
111254
[TestCase("/home/user/anycommand --argument=value&&/home/user/anyothercommand --argument2=value2", "bash -c \"/home/user/anycommand --argument=value&&/home/user/anyothercommand --argument2=value2\"")]

‎src/VirtualClient/VirtualClient.Core/Components/ExecuteCommand.cs‎

Lines changed: 101 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -125,48 +125,51 @@ protected override async Task ExecuteAsync(EventContext telemetryContext, Cancel
125125

126126
if (!cancellationToken.IsCancellationRequested)
127127
{
128-
string commandToExecute = this.GetCommandToExecute();
128+
IEnumerable<string> commandsToExecute = this.GetCommandsToExecute();
129129

130-
if (!string.IsNullOrWhiteSpace(commandToExecute))
130+
if (commandsToExecute?.Any() == true)
131131
{
132132
IDictionary<string, IConvertible> environmentVariables = this.EnvironmentVariables;
133-
if (!cancellationToken.IsCancellationRequested)
133+
foreach (string originalCommand in commandsToExecute)
134134
{
135-
if (PlatformSpecifics.TryGetCommandParts(commandToExecute, out string effectiveCommand, out string effectiveCommandArguments))
135+
if (!cancellationToken.IsCancellationRequested)
136136
{
137-
await (this.RetryPolicy ?? Policy.NoOpAsync()).ExecuteAsync(async () =>
137+
if (PlatformSpecifics.TryGetCommandParts(originalCommand, out string effectiveCommand, out string effectiveCommandArguments))
138138
{
139-
// There appears to be an unfortunate implementation choice in .NET causing a Win32Exception similar to the following when
140-
// referencing a binary and setting the working directory.
141-
//
142-
// System.ComponentModel.Win32Exception:
143-
// 'An error occurred trying to start process 'Coreinfo64.exe' with working directory 'S:\microsoft\virtualclient\out\bin\Debug\AnyCPU\VirtualClient.Main\net9.0\packages\system_tools\win-x64'.
144-
// The system cannot find the file specified.
145-
//
146-
// The .NET Process class does not reference the 'WorkingDirectory' when looking for the 'FileName' when UseShellExecute = false. The workaround
147-
// for this is to add the working directory to the PATH environment variable.
148-
string effectiveWorkingDirectory = this.WorkingDirectory;
149-
if (!string.IsNullOrWhiteSpace(effectiveWorkingDirectory))
139+
await (this.RetryPolicy ?? Policy.NoOpAsync()).ExecuteAsync(async () =>
150140
{
151-
this.PlatformSpecifics.SetEnvironmentVariable(
152-
EnvironmentVariable.PATH,
153-
effectiveWorkingDirectory,
154-
EnvironmentVariableTarget.Process,
155-
append: true);
156-
}
157-
158-
using (IProcessProxy process = this.processManager.CreateProcess(effectiveCommand, effectiveCommandArguments, effectiveWorkingDirectory))
159-
{
160-
this.AddEnvironmentVariables(process, environmentVariables);
161-
await process.StartAndWaitAsync(cancellationToken);
141+
// There appears to be an unfortunate implementation choice in .NET causing a Win32Exception similar to the following when
142+
// referencing a binary and setting the working directory.
143+
//
144+
// System.ComponentModel.Win32Exception:
145+
// 'An error occurred trying to start process 'Coreinfo64.exe' with working directory 'S:\microsoft\virtualclient\out\bin\Debug\AnyCPU\VirtualClient.Main\net9.0\packages\system_tools\win-x64'.
146+
// The system cannot find the file specified.
147+
//
148+
// The .NET Process class does not reference the 'WorkingDirectory' when looking for the 'FileName' when UseShellExecute = false. The workaround
149+
// for this is to add the working directory to the PATH environment variable.
150+
string effectiveWorkingDirectory = this.WorkingDirectory;
151+
if (!string.IsNullOrWhiteSpace(effectiveWorkingDirectory))
152+
{
153+
this.PlatformSpecifics.SetEnvironmentVariable(
154+
EnvironmentVariable.PATH,
155+
effectiveWorkingDirectory,
156+
EnvironmentVariableTarget.Process,
157+
append: true);
158+
}
162159

163-
if (!cancellationToken.IsCancellationRequested)
160+
using (IProcessProxy process = this.processManager.CreateProcess(effectiveCommand, effectiveCommandArguments, effectiveWorkingDirectory))
164161
{
165-
await this.LogProcessDetailsAsync(process, telemetryContext, logFileName: this.LogFileName);
166-
process.ThrowIfComponentOperationFailed(this.ComponentType);
162+
this.AddEnvironmentVariables(process, environmentVariables);
163+
await process.StartAndWaitAsync(cancellationToken);
164+
165+
if (!cancellationToken.IsCancellationRequested)
166+
{
167+
await this.LogProcessDetailsAsync(process, telemetryContext, logFileName: this.LogFileName);
168+
process.ThrowIfComponentOperationFailed(this.ComponentType);
169+
}
167170
}
168-
}
169-
});
171+
});
172+
}
170173
}
171174
}
172175
}
@@ -225,19 +228,13 @@ private void AddEnvironmentVariables(IProcessProxy process, IDictionary<string,
225228
}
226229
}
227230

228-
private string GetCommandToExecute()
231+
private IEnumerable<string> GetCommandsToExecute()
229232
{
230-
// There are a few command chaining scenarios we need to handle:
231-
// 1) Chaining that is not related to a shell (e.g. "execute_one.sh && execute_two.sh"). For these scenarios, we will execute these
232-
// each in sequential order as separate processes.
233-
//
234-
// 2) For cases where a shell is referenced, we will execute the entire command as a single process and let the shell handle the chaining (e.g. "bash -c 'execute_one.sh && execute_two.sh'").
235-
236-
string targetCommand = this.Command;
237233
if (this.UseShell)
238234
{
239-
// Using a shell (e.g. bash, cmd) allows support for command chaining and other shell features. The user can define the shell directly.
240-
// The 'UseShell' parameter is a flag for convenience and to support cross-platform scenarios in a single profile component definition.
235+
// When using a shell (e.g. bash, cmd), the entire command is passed to the shell as a single
236+
// process and the shell handles any chaining (e.g. &&).
237+
string targetCommand = this.Command;
241238
switch (this.Platform)
242239
{
243240
case PlatformID.Unix:
@@ -248,9 +245,70 @@ private string GetCommandToExecute()
248245
targetCommand = $"cmd /C \"{targetCommand.Trim(ExecuteCommand.Quotes)}\"";
249246
break;
250247
}
248+
249+
return new List<string> { targetCommand };
250+
}
251+
252+
// When not using a shell, split on '&&' and execute each command sequentially as separate processes.
253+
List<string> commandsToExecute = new List<string>();
254+
bool sudo = this.Command.StartsWith("sudo", StringComparison.OrdinalIgnoreCase);
255+
256+
IEnumerable<string> commands = ExecuteCommand.SplitCommands(this.Command);
257+
258+
foreach (string fullCommand in commands)
259+
{
260+
if (sudo && !fullCommand.Contains("sudo", StringComparison.OrdinalIgnoreCase))
261+
{
262+
commandsToExecute.Add($"sudo {fullCommand}");
263+
}
264+
else
265+
{
266+
commandsToExecute.Add(fullCommand);
267+
}
268+
}
269+
270+
return commandsToExecute;
271+
}
272+
273+
private static List<string> SplitCommands(string input)
274+
{
275+
var parts = new List<string>();
276+
int start = 0;
277+
char? quote = null;
278+
279+
for (int i = 0; i < input.Length; i++)
280+
{
281+
if (quote != null)
282+
{
283+
if (input[i] == quote)
284+
{
285+
quote = null;
286+
}
287+
}
288+
else if (input[i] == '"' || input[i] == '\'')
289+
{
290+
quote = input[i];
291+
}
292+
else if (input[i] == '&' && i + 1 < input.Length && input[i + 1] == '&')
293+
{
294+
string part = input.Substring(start, i - start).Trim();
295+
if (part.Length > 0)
296+
{
297+
parts.Add(part);
298+
}
299+
300+
i++;
301+
start = i + 1;
302+
}
303+
}
304+
305+
string last = input.Substring(start).Trim();
306+
if (last.Length > 0)
307+
{
308+
parts.Add(last);
251309
}
252310

253-
return targetCommand;
311+
return parts;
254312
}
255313
}
256314
}

0 commit comments

Comments
 (0)