shwstppr commented on code in PR #11947:
URL: https://github.com/apache/cloudstack/pull/11947#discussion_r2486095630
##########
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)};
Review Comment:
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
--
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]
