Skip to content

[VMware to KVM Migration] Fix unused convert env vars #11947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+32 −7
Open

[VMware to KVM Migration] Fix unused convert env vars #11947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .../java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.UUID;

import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.StringUtils;

import com.cloud.agent.api.Answer;
Expand Down Expand Up @@ -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);
}
Comment on lines +249 to +253
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (ArrayUtils.isEmpty(convertInstanceEnv)) {
script.execute(outputLogger);
} else {
script.execute(outputLogger, convertInstanceEnv);
}
script.execute(outputLogger, convertInstanceEnv);

Copilot uses AI. Check for mistakes.
int exitValue = script.getExitValue();
return exitValue == 0;
}
Expand Down
31 changes: 25 additions & 6 deletions utils/src/main/java/com/cloud/utils/script/Script.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@

import org.apache.cloudstack.utils.security.KeyStoreUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.joda.time.Duration;
Expand Down Expand Up @@ -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) {
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
public String executeInternal(OutputInterpreter interpreter, String[] environment) {
private String executeInternal(OutputInterpreter interpreter, String[] environment) {

Copilot uses AI. Check for mistakes.
Comment on lines +243 to +247
Copy link
Contributor

@DaanHoogland DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public String execute(OutputInterpreter interpreter, String[] environment) {
return executeInternal(interpreter, environment);
}
public String executeInternal(OutputInterpreter interpreter, String[] environment) {
public String execute(OutputInterpreter interpreter, String[] environment) {

no need for the extra call on the stack, that I can see

String[] command = _command.toArray(new String[_command.size()]);
String commandLine = buildCommandLine(command);
if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
Expand All @@ -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)};
Copy link
Contributor

@shwstppr shwstppr Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using shell invocation may be comparatively unsafe versus ProcessBuilder. Not sure if possible to avoid this?
No log is added for this else block, may help

Javadoc for new execute overloaded method may help

Copy link
Contributor

@DaanHoogland DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, co-pilots comments are giving some hints.

// 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")));
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));
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.");
}

Copilot uses AI. Check for mistakes.
_process = Runtime.getRuntime().exec(commands, env, _workDir != null ? new File(_workDir) : null);
Comment on lines +266 to +271
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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);
// 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();

Copilot uses AI. Check for mistakes.
}

_logger.trace(String.format("Starting process for command [%s].", commandLine));
_process = pb.start();
if (_process == null) {
_logger.warn(String.format("Unable to execute command [%s] because no process was created.", commandLine));
return "Unable to execute the command: " + command[0];
Expand Down
Loading