From 81bfd3a02b9ad67ca1a3ebfb28033d2aece22602 Mon Sep 17 00:00:00 2001 From: Asaf Gabai <77976014+asafgabai@users.noreply.github.com> Date: Mon, 15 Jan 2024 16:16:38 +0200 Subject: [PATCH] Fix running executables with spaces in their paths in Windows (#777) --- .../jfrog/build/extractor/go/GoDriver.java | 46 +-------- .../build/extractor/go/GoDriverTest.java | 19 ---- .../extractor/executor/CommandExecutor.java | 99 ++++++++++++++----- .../executor/CommandExecutorTest.java | 21 ++++ 4 files changed, 96 insertions(+), 89 deletions(-) diff --git a/build-info-extractor-go/src/main/java/org/jfrog/build/extractor/go/GoDriver.java b/build-info-extractor-go/src/main/java/org/jfrog/build/extractor/go/GoDriver.java index 1cc8aad99..89d412a93 100644 --- a/build-info-extractor-go/src/main/java/org/jfrog/build/extractor/go/GoDriver.java +++ b/build-info-extractor-go/src/main/java/org/jfrog/build/extractor/go/GoDriver.java @@ -1,8 +1,6 @@ package org.jfrog.build.extractor.go; -import com.google.common.collect.Maps; import org.apache.commons.lang3.StringUtils; -import org.apache.commons.lang3.SystemUtils; import org.jfrog.build.api.util.Log; import org.jfrog.build.extractor.executor.CommandExecutor; import org.jfrog.build.extractor.executor.CommandResults; @@ -26,7 +24,6 @@ public class GoDriver implements Serializable { private static final String GO_GET_CMD = "get"; private static final String GO_LIST_MODULE_CMD = "list -m"; private static final String GO_VERSION_CMD = "version"; - private static final String DEFAULT_EXECUTABLE_NAME = "go"; private static final long serialVersionUID = 1L; private final CommandExecutor commandExecutor; @@ -34,52 +31,11 @@ public class GoDriver implements Serializable { private final Log logger; public GoDriver(String executablePath, Map env, File workingDirectory, Log logger) { - this.commandExecutor = generateCommandExecutor(executablePath, env); + this.commandExecutor = new CommandExecutor(StringUtils.defaultIfEmpty(executablePath, "go"), env); this.workingDirectory = workingDirectory; this.logger = logger; } - /** - * Generate a new mutable copy of environment variables map with the Go executable directory path inserted to the beginning of the Path. - * - * @param executablePath Go executable path - * @param env Environment variables map - * @return a new Environment variables map - */ - static Map generateWindowsEnv(String executablePath, Map env) { - // If executablePath ends with "go" or "go.exe" - remove it from the directory path - executablePath = StringUtils.removeEnd(executablePath, ".exe"); - executablePath = StringUtils.removeEnd(executablePath, DEFAULT_EXECUTABLE_NAME); - - // Insert the Go executable directory path to the beginning of the Path environment variable - // Make sure to copy the environment variables map to avoid changing the original map or in case it is immutable - Map newEnv = Maps.newHashMap(env); - String windowsPathEnvKey = "Path"; - if (newEnv.containsKey(windowsPathEnvKey)) { - newEnv.put(windowsPathEnvKey, executablePath + File.pathSeparator + newEnv.get(windowsPathEnvKey)); - } else { - newEnv.put(windowsPathEnvKey, executablePath); - } - return newEnv; - } - - /** - * Create a CommandExecutor with the given executable path and environment variables. - * - * @param executablePath Go executable path - * @param env Environment variables map - * @return CommandExecutor - */ - private static CommandExecutor generateCommandExecutor(String executablePath, Map env) { - if (!SystemUtils.IS_OS_WINDOWS || StringUtils.isBlank(executablePath) || StringUtils.equals(executablePath, DEFAULT_EXECUTABLE_NAME) || env == null) { - return new CommandExecutor(StringUtils.defaultIfEmpty(executablePath, DEFAULT_EXECUTABLE_NAME), env); - } - // Handling Windows case with a given executable path: - // A bug was identified for the Go executable in Windows where the executable path may be incorrectly parsed - // as two command arguments when the path contains spaces (e.g., "C:\Program Files\Go\bin\go.exe"). - return new CommandExecutor(DEFAULT_EXECUTABLE_NAME, generateWindowsEnv(executablePath, env)); - } - public CommandResults runCmd(String args, boolean verbose) throws IOException { List argsList = new ArrayList<>(Arrays.asList(args.split(" "))); return runCmd(argsList, verbose); diff --git a/build-info-extractor-go/src/test/java/org/jfrog/build/extractor/go/GoDriverTest.java b/build-info-extractor-go/src/test/java/org/jfrog/build/extractor/go/GoDriverTest.java index 5acc9ff13..eaa77580c 100644 --- a/build-info-extractor-go/src/test/java/org/jfrog/build/extractor/go/GoDriverTest.java +++ b/build-info-extractor-go/src/test/java/org/jfrog/build/extractor/go/GoDriverTest.java @@ -2,11 +2,9 @@ import com.google.common.collect.Sets; import org.apache.commons.io.FileUtils; -import org.apache.commons.lang3.SystemUtils; import org.jfrog.build.api.util.NullLog; import org.jfrog.build.extractor.executor.CommandResults; import org.testng.Assert; -import org.testng.SkipException; import org.testng.annotations.Test; import java.io.File; @@ -15,7 +13,6 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Arrays; -import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -117,20 +114,4 @@ public void testGoGet() throws IOException { FileUtils.deleteDirectory(projectDir); } } - - @Test - public void testGoDriverWindowsInit() throws IOException { - if (!SystemUtils.IS_OS_WINDOWS) { - throw new SkipException("Skipping non-windows test"); - } - File projectDir = Files.createTempDirectory("").toFile(); - try { - Map systemEnv = System.getenv(); - String executablePath = "C:\\Program Files\\Go\\bin\\go"; - Map executorEnv = GoDriver.generateWindowsEnv(executablePath, systemEnv); - assertTrue(executorEnv.get("Path").startsWith("C:\\Program Files\\Go\\bin")); - } finally { - FileUtils.deleteDirectory(projectDir); - } - } } diff --git a/build-info-extractor/src/main/java/org/jfrog/build/extractor/executor/CommandExecutor.java b/build-info-extractor/src/main/java/org/jfrog/build/extractor/executor/CommandExecutor.java index e27217e84..2d5161e8b 100644 --- a/build-info-extractor/src/main/java/org/jfrog/build/extractor/executor/CommandExecutor.java +++ b/build-info-extractor/src/main/java/org/jfrog/build/extractor/executor/CommandExecutor.java @@ -1,5 +1,6 @@ package org.jfrog.build.extractor.executor; +import com.google.common.collect.Maps; import org.apache.commons.lang3.SystemUtils; import org.jfrog.build.api.util.Log; import org.jfrog.build.extractor.UrlUtils; @@ -8,11 +9,12 @@ import java.io.IOException; import java.io.InputStream; import java.io.Serializable; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.*; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; import static java.lang.String.format; import static java.lang.String.join; @@ -26,7 +28,7 @@ public class CommandExecutor implements Serializable { private static final int READER_SHUTDOWN_TIMEOUT_SECONDS = 30; private static final int PROCESS_TERMINATION_TIMEOUT_SECONDS = 30; private static final int EXECUTION_TIMEOUT_MINUTES = 120; - private Map env; + private final Map env; private final String executablePath; /** @@ -34,7 +36,7 @@ public class CommandExecutor implements Serializable { * @param env - Environment variables to use during execution. */ public CommandExecutor(String executablePath, Map env) { - this.executablePath = escapeSpacesInPath(executablePath); + this.executablePath = executablePath.trim(); Map finalEnvMap = new HashMap<>(System.getenv()); if (env != null) { Map fixedEnvMap = new HashMap<>(env); @@ -133,10 +135,9 @@ public CommandResults exeCommand(File execDir, List args, List c */ public CommandResults exeCommand(File execDir, List args, List credentials, Log logger, long timeout, TimeUnit unit) throws InterruptedException, IOException { List command = new ArrayList<>(args); - command.add(0, executablePath); ExecutorService service = Executors.newFixedThreadPool(2); try { - Process process = runProcess(execDir, command, credentials, env, logger); + Process process = runProcess(execDir, executablePath, command, credentials, env, logger); // The output stream is not necessary in non-interactive scenarios, therefore we can close it now. process.getOutputStream().close(); try (InputStream inputStream = process.getInputStream(); InputStream errorStream = process.getErrorStream()) { @@ -179,38 +180,86 @@ private CommandResults getCommandResults(boolean terminatedProperly, List args, List credentials, Map env, Log logger) throws IOException { + private static Process runProcess(File execDir, String executablePath, List args, List credentials, Map env, Log logger) throws IOException { + // Make sure to copy the environment variables map to avoid changing the original map or in case it is immutable. + Map newEnv = Maps.newHashMap(env); + + args = formatCommand(args, credentials, executablePath, newEnv); + logCommand(logger, args, credentials); + ProcessBuilder processBuilder = new ProcessBuilder(args) + .directory(execDir); + processBuilder.environment().putAll(newEnv); + return processBuilder.start(); + } + + /** + * Formats a command for execution, incorporating credentials and environment variables. + * + * @param args the list of arguments to be included in the command + * @param credentials if specified, the credentials will be concatenated to the command + * @param executablePath the path to the executable to be executed + * @param env environment variables map. It might be modified as part of the formatting process + * @return the formatted command as a list of strings, ready for execution + */ + private static List formatCommand(List args, List credentials, String executablePath, Map env) { if (credentials != null) { args.addAll(credentials); } + if (SystemUtils.IS_OS_WINDOWS) { - args.addAll(0, Arrays.asList("cmd", "/c")); + formatWindowsCommand(args, executablePath, env); + return args; + } + return formatUnixCommand(args, executablePath); + } + + /** + * Formats a Windows command for execution. + * + * @param args the list of arguments to be included in the command + * @param executablePath the path to the executable to be executed + * @param env environment variables map. It might be modified as part of the formatting process + */ + private static void formatWindowsCommand(List args, String executablePath, Map env) { + Path execPath = Paths.get(executablePath); + if (execPath.isAbsolute()) { + addToWindowsPath(env, execPath); + args.add(0, execPath.getFileName().toString()); } else { - String strArgs = join(" ", args); - args = new ArrayList() {{ - add("/bin/sh"); - add("-c"); - add(strArgs); - }}; + args.add(0, executablePath.replaceAll(" ", "^ ")); } - logCommand(logger, args, credentials); - ProcessBuilder processBuilder = new ProcessBuilder(args) - .directory(execDir); - processBuilder.environment().putAll(env); - return processBuilder.start(); + args.addAll(0, Arrays.asList("cmd", "/c")); + } + + private static List formatUnixCommand(List args, String executablePath) { + args.add(0, executablePath.replaceAll(" ", "\\\\ ")); + String strArgs = join(" ", args); + return new ArrayList() {{ + add("/bin/sh"); + add("-c"); + add(strArgs); + }}; } /** - * Escape spaces in the input executable path and trim leading and trailing whitespaces. + * Inserts the executable directory path at the beginning of the Path environment variable. + * This is done to handle cases where the executable path contains spaces. In such scenarios, the "cmd" command used + * to execute this command in Windows may incorrectly parse the path, treating the section after the space as an + * argument for the command. * - * @param executablePath - the executable path to process - * @return escaped and trimmed executable path. + * @param env environment variables map + * @param execPath the executable path */ - private static String escapeSpacesInPath(String executablePath) { - if (executablePath == null) { - return null; + static void addToWindowsPath(Map env, Path execPath) { + String execDirPath = execPath.getParent().toString(); + + // Insert the executable directory path to the beginning of the Path environment variable. + String windowsPathEnvKey = "Path"; + if (env.containsKey(windowsPathEnvKey)) { + env.put(windowsPathEnvKey, execDirPath + File.pathSeparator + env.get(windowsPathEnvKey)); + } else { + env.put(windowsPathEnvKey, execDirPath); } - return executablePath.trim().replaceAll(" ", SystemUtils.IS_OS_WINDOWS ? "^ " : "\\\\ "); } private static void logCommand(Log logger, List args, List credentials) { diff --git a/build-info-extractor/src/test/java/org/jfrog/build/extractor/executor/CommandExecutorTest.java b/build-info-extractor/src/test/java/org/jfrog/build/extractor/executor/CommandExecutorTest.java index 2a3723d12..c5cd361b2 100644 --- a/build-info-extractor/src/test/java/org/jfrog/build/extractor/executor/CommandExecutorTest.java +++ b/build-info-extractor/src/test/java/org/jfrog/build/extractor/executor/CommandExecutorTest.java @@ -1,8 +1,11 @@ package org.jfrog.build.extractor.executor; +import com.google.common.collect.Maps; import org.apache.commons.io.FileUtils; +import org.apache.commons.lang3.SystemUtils; import org.apache.commons.lang3.exception.ExceptionUtils; import org.jfrog.build.api.util.NullLog; +import org.testng.SkipException; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import org.testng.collections.Lists; @@ -12,8 +15,10 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.List; +import java.util.Map; import static org.testng.Assert.*; @@ -85,4 +90,20 @@ public void testExeCommandWithSpaces() throws IOException, InterruptedException FileUtils.forceDelete(tmpDir.toFile()); } } + + @Test + public void testGenerateWindowsEnv() throws IOException { + if (!SystemUtils.IS_OS_WINDOWS) { + throw new SkipException("Skipping test on non-Windows OS"); + } + File projectDir = Files.createTempDirectory("").toFile(); + try { + Map env = Maps.newHashMap(System.getenv()); + Path execPath = Paths.get("C:\\Program Files\\Go\\bin\\go"); + CommandExecutor.addToWindowsPath(env, execPath); + assertTrue(env.get("Path").startsWith("C:\\Program Files\\Go\\bin")); + } finally { + FileUtils.deleteDirectory(projectDir); + } + } }