Copilot commented on code in PR #11947:
URL: https://github.com/apache/cloudstack/pull/11947#discussion_r2486093296
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -236,6 +237,14 @@ static String stackTraceAsString(Throwable throwable) {
}
public String execute(OutputInterpreter interpreter) {
+ return execute(interpreter, null);
+ }
+
+ public String execute(OutputInterpreter interpreter, String[] environment)
{
+ return executeInternal(interpreter, environment);
+ }
+
+ public String executeInternal(OutputInterpreter interpreter, String[]
environment) {
Review Comment:
The `executeInternal` method is public but appears to be an internal
implementation detail. Consider making this method private or package-private
to prevent direct external usage, as callers should use the `execute()` methods
instead.
```suggestion
private String executeInternal(OutputInterpreter interpreter, String[]
environment) {
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -245,13 +254,23 @@ public String execute(OutputInterpreter interpreter) {
try {
_logger.trace(String.format("Creating process for command [%s].",
commandLine));
- ProcessBuilder pb = new ProcessBuilder(command);
- pb.redirectErrorStream(true);
- if (_workDir != null)
- pb.directory(new File(_workDir));
+ if (ArrayUtils.isEmpty(environment)) {
+ ProcessBuilder pb = new ProcessBuilder(command);
+ pb.redirectErrorStream(true);
+ if (_workDir != null)
+ pb.directory(new File(_workDir));
+
+ _logger.trace(String.format("Starting process for command
[%s].", commandLine));
+ _process = pb.start();
+ } else {
+ // Since Runtime.exec() does not support redirecting the error
stream, then append 2>&1 to the command
+ String[] commands = new String[] {"sh", "-c",
String.format("%s 2>&1", commandLine)};
+ // The PATH variable must be added for indirect calls within
the running command
+ // Example: virt-v2v invokes qemu-img, which cannot be found
if PATH is not set
+ String[] env = ArrayUtils.add(environment,
String.format("PATH=%s", System.getenv("PATH")));
+ _process = Runtime.getRuntime().exec(commands, env, _workDir
!= null ? new File(_workDir) : null);
Review Comment:
Wrapping the command in a shell (`sh -c`) could introduce command injection
vulnerabilities if `commandLine` contains untrusted input. Ensure that all
components of `_command` are properly validated or sanitized before being
passed to this method, or consider using ProcessBuilder with proper environment
variable support instead.
```suggestion
// Use ProcessBuilder to set environment variables and
redirect error stream safely
ProcessBuilder pb = new ProcessBuilder(command);
pb.redirectErrorStream(true);
if (_workDir != null)
pb.directory(new File(_workDir));
// Set environment variables
for (String envVar : environment) {
int idx = envVar.indexOf('=');
if (idx > 0) {
String key = envVar.substring(0, idx);
String value = envVar.substring(idx + 1);
pb.environment().put(key, value);
}
}
// Ensure PATH is set
pb.environment().put("PATH", System.getenv("PATH"));
_process = pb.start();
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -245,13 +254,23 @@ public String execute(OutputInterpreter interpreter) {
try {
_logger.trace(String.format("Creating process for command [%s].",
commandLine));
- ProcessBuilder pb = new ProcessBuilder(command);
- pb.redirectErrorStream(true);
- if (_workDir != null)
- pb.directory(new File(_workDir));
+ if (ArrayUtils.isEmpty(environment)) {
+ ProcessBuilder pb = new ProcessBuilder(command);
+ pb.redirectErrorStream(true);
+ if (_workDir != null)
+ pb.directory(new File(_workDir));
+
+ _logger.trace(String.format("Starting process for command
[%s].", commandLine));
+ _process = pb.start();
+ } else {
+ // Since Runtime.exec() does not support redirecting the error
stream, then append 2>&1 to the command
+ String[] commands = new String[] {"sh", "-c",
String.format("%s 2>&1", commandLine)};
+ // The PATH variable must be added for indirect calls within
the running command
+ // Example: virt-v2v invokes qemu-img, which cannot be found
if PATH is not set
+ String[] env = ArrayUtils.add(environment,
String.format("PATH=%s", System.getenv("PATH")));
Review Comment:
If `System.getenv(\"PATH\")` returns null (which can happen in some
environments), this will add \"PATH=null\" to the environment variables,
potentially breaking command execution. Add a null check and handle the case
where PATH is not defined.
```suggestion
String pathEnv = System.getenv("PATH");
String[] env = environment;
if (pathEnv != null) {
env = ArrayUtils.add(environment,
String.format("PATH=%s", pathEnv));
} else {
_logger.warn("System environment variable PATH is not
set; indirect command calls may fail.");
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -244,7 +245,12 @@ protected boolean performInstanceConversion(String
originalVMName, String source
String logPrefix = String.format("(%s) virt-v2v ovf source: %s
progress", originalVMName, sourceOVFDirPath);
OutputInterpreter.LineByLineOutputLogger outputLogger = new
OutputInterpreter.LineByLineOutputLogger(logger, logPrefix);
- script.execute(outputLogger);
+ String[] convertInstanceEnv = serverResource.getConvertInstanceEnv();
+ if (ArrayUtils.isEmpty(convertInstanceEnv)) {
+ script.execute(outputLogger);
+ } else {
+ script.execute(outputLogger, convertInstanceEnv);
+ }
Review Comment:
The conditional check and branching can be simplified. The
`execute(outputLogger)` method already calls `execute(outputLogger, null)`, and
the implementation handles null/empty arrays. You can directly call
`script.execute(outputLogger, convertInstanceEnv)` without the conditional
check, simplifying the code.
```suggestion
script.execute(outputLogger, convertInstanceEnv);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
