diff --git a/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/ProcrunCommand.java b/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/ProcrunCommand.java index c10495d3b8af6..b507e5e43a456 100644 --- a/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/ProcrunCommand.java +++ b/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/ProcrunCommand.java @@ -67,7 +67,7 @@ protected void execute(Terminal terminal, OptionSet options, ProcessInfo process preExecute(terminal, processInfo, serviceId); List procrunCmd = new ArrayList<>(); - procrunCmd.add(procrun.toString()); + procrunCmd.add(quote(procrun.toString())); procrunCmd.add("//%s/%s".formatted(cmd, serviceId)); if (includeLogArgs()) { procrunCmd.add(getLogArgs(serviceId, processInfo.workingDir(), processInfo.envVars())); @@ -86,6 +86,11 @@ protected void execute(Terminal terminal, OptionSet options, ProcessInfo process } } + /** Quotes the given String. */ + static String quote(String s) { + return '"' + s + '"'; + } + /** Determines the service id for the Elasticsearch service that should be used */ private String getServiceId(OptionSet options, Map env) throws UserException { List args = options.nonOptionArguments(); diff --git a/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/WindowsServiceInstallCommand.java b/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/WindowsServiceInstallCommand.java index 4e6e2cddfeb93..0d0bd040db30a 100644 --- a/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/WindowsServiceInstallCommand.java +++ b/distribution/tools/windows-service-cli/src/main/java/org/elasticsearch/windows/service/WindowsServiceInstallCommand.java @@ -42,7 +42,7 @@ protected String getAdditionalArgs(String serviceId, ProcessInfo pinfo) { addArg(args, "--Classpath", pinfo.sysprops().get("java.class.path")); addArg(args, "--JvmMs", "4m"); addArg(args, "--JvmMx", "64m"); - addArg(args, "--JvmOptions", getJvmOptions(pinfo.sysprops())); + addQuotedArg(args, "--JvmOptions", getJvmOptions(pinfo.sysprops())); addArg(args, "--PidFile", "%s.pid".formatted(serviceId)); addArg( args, @@ -55,10 +55,10 @@ protected String getAdditionalArgs(String serviceId, ProcessInfo pinfo) { pinfo.envVars() .getOrDefault("SERVICE_DESCRIPTION", "Elasticsearch %s Windows Service - https://elastic.co".formatted(Version.CURRENT)) ); - addArg(args, "--Jvm", getJvmDll(getJavaHome(pinfo.sysprops())).toString()); + addQuotedArg(args, "--Jvm", quote(getJvmDll(getJavaHome(pinfo.sysprops())).toString())); addArg(args, "--StartMode", "jvm"); addArg(args, "--StopMode", "jvm"); - addArg(args, "--StartPath", pinfo.workingDir().toString()); + addQuotedArg(args, "--StartPath", quote(pinfo.workingDir().toString())); addArg(args, "++JvmOptions", "-Dcli.name=windows-service-daemon"); addArg(args, "++JvmOptions", "-Dcli.libs=lib/tools/server-cli,lib/tools/windows-service-cli"); addArg(args, "++Environment", "HOSTNAME=%s".formatted(pinfo.envVars().get("COMPUTERNAME"))); @@ -89,6 +89,13 @@ private static void addArg(List args, String arg, String value) { args.add(value); } + // Adds an arg with an already appropriately quoted value. Trivial, but explicit implementation. + // This method is typically used when adding args whose value contains a file-system path + private static void addQuotedArg(List args, String arg, String value) { + args.add(arg); + args.add(value); + } + @SuppressForbidden(reason = "get java home path to pass through") private static Path getJavaHome(Map sysprops) { return Paths.get(sysprops.get("java.home")); @@ -107,7 +114,7 @@ private static String getJvmOptions(Map sysprops) { jvmOptions.add("-XX:+UseSerialGC"); // passthrough these properties for (var prop : List.of("es.path.home", "es.path.conf", "es.distribution.type")) { - jvmOptions.add("-D%s=%s".formatted(prop, sysprops.get(prop))); + jvmOptions.add("-D%s=%s".formatted(prop, quote(sysprops.get(prop)))); } return String.join(";", jvmOptions); } diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/ProcrunCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/ProcrunCommandTests.java index b683884a37571..e4b651fcb77af 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/ProcrunCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/ProcrunCommandTests.java @@ -25,6 +25,10 @@ public class ProcrunCommandTests extends WindowsServiceCliTestCase { + public ProcrunCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + PreExecuteHook preExecuteHook; boolean includeLogArgs; String additionalArgs; diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceCliTestCase.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceCliTestCase.java index b727774ea2d1d..808173005b96f 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceCliTestCase.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceCliTestCase.java @@ -8,6 +8,8 @@ package org.elasticsearch.windows.service; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; + import org.elasticsearch.cli.CommandTestCase; import org.junit.Before; @@ -47,6 +49,15 @@ public abstract class WindowsServiceCliTestCase extends CommandTestCase { int mockProcessExit = 0; ProcessValidator mockProcessValidator = null; + @ParametersFactory + public static Iterable spaceInPathProvider() { + return List.of(new Object[] { true }, new Object[] { false }); + } + + protected WindowsServiceCliTestCase(boolean spaceInPath) { + super(spaceInPath); + } + interface ProcessValidator { void validate(Map env, ProcrunCall procrunCall); } @@ -106,16 +117,22 @@ protected Process mockProcess(ProcessBuilder processBuilder) throws IOException private static final Pattern commandPattern = Pattern.compile("//([A-Z]{2})/([\\w-]+)"); private static ProcrunCall parseProcrunCall(String unparsedArgs) { + // command/exe is quoted + assert unparsedArgs.charAt(0) == '"'; + int idx = unparsedArgs.indexOf('"', 1); + String exe = unparsedArgs.substring(0, idx + 1); + // Strip the leading command/exe from the args + unparsedArgs = unparsedArgs.substring(idx + 1).stripLeading(); + String[] splitArgs = unparsedArgs.split(" "); - assertThat(unparsedArgs, splitArgs.length, greaterThanOrEqualTo(2)); + assertThat(unparsedArgs, splitArgs.length, greaterThanOrEqualTo(1)); Map> args = new HashMap<>(); - String exe = splitArgs[0]; - Matcher commandMatcher = commandPattern.matcher(splitArgs[1]); - assertThat(splitArgs[1], commandMatcher.matches(), is(true)); + Matcher commandMatcher = commandPattern.matcher(splitArgs[0]); + assertThat(splitArgs[0], commandMatcher.matches(), is(true)); String command = commandMatcher.group(1); String serviceId = commandMatcher.group(2); - int i = 2; + int i = 1; while (i < splitArgs.length) { String arg = splitArgs[i]; assertThat("procrun args begin with -- or ++", arg, anyOf(startsWith("--"), startsWith("++"))); @@ -165,8 +182,12 @@ public void resetMockProcess() throws Exception { protected abstract String getDefaultFailureMessage(); + static String quote(String s) { + return '"' + s + '"'; + } + protected String getExe() { - return serviceExe.toString(); + return quote(serviceExe.toString()); } protected boolean includeLogsArgs() { diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceInstallCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceInstallCommandTests.java index ffd0e16fd6f79..0db531074498f 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceInstallCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceInstallCommandTests.java @@ -31,6 +31,10 @@ public class WindowsServiceInstallCommandTests extends WindowsServiceCliTestCase Path jvmDll; + public WindowsServiceInstallCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + @Before public void setupJvm() throws Exception { jvmDll = javaHome.resolve("jre/bin/server/jvm.dll"); @@ -80,7 +84,7 @@ public void testAlternateDllLocation() throws Exception { } public void testDll() throws Exception { - assertServiceArgs(Map.of("Jvm", jvmDll.toString())); + assertServiceArgs(Map.of("Jvm", quote(jvmDll.toString()))); } public void testPreExecuteOutput() throws Exception { @@ -95,9 +99,9 @@ public void testJvmOptions() throws Exception { sysprops.put("es.distribution.type", "testdistro"); List expectedOptions = List.of( "" + "-XX:+UseSerialGC", - "-Des.path.home=" + esHomeDir.toString(), - "-Des.path.conf=" + esHomeDir.resolve("config").toString(), - "-Des.distribution.type=testdistro" + "-Des.path.home=" + quote(esHomeDir.toString()), + "-Des.path.conf=" + quote(esHomeDir.resolve("config").toString()), + "-Des.distribution.type=" + quote("testdistro") ); mockProcessValidator = (environment, procrunCall) -> { List options = procrunCall.args().get("JvmOptions"); @@ -136,7 +140,7 @@ public void testFixedArgs() throws Exception { entry("StopMode", "jvm"), entry("JvmMs", "4m"), entry("JvmMx", "64m"), - entry("StartPath", esHomeDir.toString()), + entry("StartPath", quote(esHomeDir.toString())), entry("Classpath", "javaclasspath") // dummy value for tests ) ); diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceManagerCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceManagerCommandTests.java index cd3aea949f0f6..1699dd3f78316 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceManagerCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceManagerCommandTests.java @@ -13,6 +13,11 @@ import java.io.IOException; public class WindowsServiceManagerCommandTests extends WindowsServiceCliTestCase { + + public WindowsServiceManagerCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + @Override protected Command newCommand() { return new WindowsServiceManagerCommand() { @@ -25,7 +30,7 @@ Process startProcess(ProcessBuilder processBuilder) throws IOException { @Override protected String getExe() { - return mgrExe.toString(); + return quote(mgrExe.toString()); } @Override diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceRemoveCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceRemoveCommandTests.java index 3d2032d75a195..d0e72e9de5c66 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceRemoveCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceRemoveCommandTests.java @@ -13,6 +13,11 @@ import java.io.IOException; public class WindowsServiceRemoveCommandTests extends WindowsServiceCliTestCase { + + public WindowsServiceRemoveCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + @Override protected Command newCommand() { return new WindowsServiceRemoveCommand() { diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStartCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStartCommandTests.java index 7a30540d53ba0..502008d22422f 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStartCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStartCommandTests.java @@ -13,6 +13,11 @@ import java.io.IOException; public class WindowsServiceStartCommandTests extends WindowsServiceCliTestCase { + + public WindowsServiceStartCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + @Override protected Command newCommand() { return new WindowsServiceStartCommand() { diff --git a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStopCommandTests.java b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStopCommandTests.java index f623c5d2465f3..a36e090bd7ac4 100644 --- a/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStopCommandTests.java +++ b/distribution/tools/windows-service-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStopCommandTests.java @@ -13,6 +13,11 @@ import java.io.IOException; public class WindowsServiceStopCommandTests extends WindowsServiceCliTestCase { + + public WindowsServiceStopCommandTests(boolean spaceInPath) { + super(spaceInPath); + } + @Override protected Command newCommand() { return new WindowsServiceStopCommand() { diff --git a/docs/changelog/89072.yaml b/docs/changelog/89072.yaml new file mode 100644 index 0000000000000..6647c892141d0 --- /dev/null +++ b/docs/changelog/89072.yaml @@ -0,0 +1,6 @@ +pr: 89072 +summary: Quote paths with whitespace in Windows service CLIs +area: Infra/CLI +type: bug +issues: + - 89043 diff --git a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java index 09e07f32d820a..6c3573e2594d3 100644 --- a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java @@ -41,12 +41,27 @@ public abstract class CommandTestCase extends ESTestCase { /** The ES config dir */ protected Path configDir; + /** Whether to include a whitespace in the file-system path. */ + private final boolean spaceInPath; + + protected CommandTestCase() { + this(false); + } + + protected CommandTestCase(boolean spaceInPath) { + this.spaceInPath = spaceInPath; + } + @Before public void resetTerminal() throws IOException { terminal.reset(); terminal.setSupportsBinary(false); terminal.setVerbosity(Terminal.Verbosity.NORMAL); - esHomeDir = createTempDir(); + if (spaceInPath) { + esHomeDir = createTempDir("a b"); // contains a whitespace + } else { + esHomeDir = createTempDir(); + } configDir = esHomeDir.resolve("config"); Files.createDirectory(configDir); sysprops.clear();