Quote paths with whitespace in Windows service CLIs (elastic#89072)

NOUIY · Aug 8, 2022 · ac25477 · ac25477
1 parent 81265d2
commit ac25477
Show file tree

Hide file tree

Showing 11 changed files with 100 additions and 18 deletions.
diff --git a/...s/windows-service-cli/src/main/java/org/elasticsearch/windows/service/ProcrunCommand.java b/...s/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<String> 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<String, String> env) throws UserException {
         List<?> args = options.nonOptionArguments();

diff --git a/...ice-cli/src/main/java/org/elasticsearch/windows/service/WindowsServiceInstallCommand.java b/...ice-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<String> 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<String> 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<String, String> sysprops) {
         return Paths.get(sysprops.get("java.home"));
@@ -107,7 +114,7 @@ private static String getJvmOptions(Map<String, String> 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/...dows-service-cli/src/test/java/org/elasticsearch/windows/service/ProcrunCommandTests.java b/...dows-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/...ervice-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceCliTestCase.java b/...ervice-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<Object[]> spaceInPathProvider() {
+        return List.of(new Object[] { true }, new Object[] { false });
+    }
+
+    protected WindowsServiceCliTestCase(boolean spaceInPath) {
+        super(spaceInPath);
+    }
+
     interface ProcessValidator {
         void validate(Map<String, String> 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<String, List<String>> 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/...li/src/test/java/org/elasticsearch/windows/service/WindowsServiceInstallCommandTests.java b/...li/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<String> 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<String> 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/...li/src/test/java/org/elasticsearch/windows/service/WindowsServiceManagerCommandTests.java b/...li/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/...cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceRemoveCommandTests.java b/...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/...-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStartCommandTests.java b/...-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/...e-cli/src/test/java/org/elasticsearch/windows/service/WindowsServiceStopCommandTests.java b/...e-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
@@ -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
@@ -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();