Fixes #9999 and #10055 - proper quoting for dry-run

Using "strong quoting" with the single quote character to quote argument of --dry-run output.

Signed-off-by: gregw <gregw@webtide.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

@@ -16,6 +16,7 @@ package org.eclipse.jetty.start;
 import java.io.File;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Objects;
 
 public class CommandLineBuilder
 {
@@ -54,12 +55,9 @@ public class CommandLineBuilder
     }
 
     /**
-     * Perform an optional quoting of the argument, being intelligent with spaces and quotes as needed. If a subString is set in quotes it won't the subString
+     * @param arg string
-     * won't be escaped.
+     * @return Quoted string
-     *
+     * @deprecated use {@link #shellQuoteIfNeeded(String)}
-     * @param arg the argument to quote
-     * @return the quoted and escaped argument
-     * @deprecated no replacement, quoting is done by {@link #toQuotedString()} now.
      */
     @Deprecated
     public static String quote(String arg)
@@ -67,23 +65,97 @@ public class CommandLineBuilder
         return "'" + arg + "'";
     }
 
-    private List<String> args;
+    private final StringBuilder commandLine = new StringBuilder();
+    private final List<String> args = new ArrayList<>();
+    private final String separator;
 
     public CommandLineBuilder()
     {
-        args = new ArrayList<String>();
+        this(false);
     }
 
+    @Deprecated
     public CommandLineBuilder(String bin)
     {
         this();
         args.add(bin);
     }
 
+    public CommandLineBuilder(boolean multiline)
+    {
+        separator = multiline ? (" \\" + System.lineSeparator() + "  ") : " ";
+    }
+
     /**
-     * Add a simple argument to the command line.
+     * This method applies single quotes suitable for a POSIX compliant shell if
-     * <p>
+     * necessary.
-     * Will quote arguments that have a space in them.
+     *
+     * @param input The string to quote if needed
+     * @return The quoted string or the original string if quotes are not necessary
+     */
+    public static String shellQuoteIfNeeded(String input)
+    {
+        // Single quotes are used because double quotes
+        // are evaluated differently by some shells.
+
+        if (input == null)
+            return null;
+        if (input.length() == 0)
+            return "''";
+
+        int i = 0;
+        boolean needsQuoting = false;
+        while (!needsQuoting && i < input.length())
+        {
+            char c = input.charAt(i++);
+
+            // needs quoting unless a limited set of known good characters
+            needsQuoting = !(
+                (c >= 'A' && c <= 'Z') ||
+                (c >= 'a' && c <= 'z') ||
+                (c >= '0' && c <= '9') ||
+                c == '/' ||
+                c == ':' ||
+                c == '.' ||
+                c == ',' ||
+                c == '+' ||
+                c == '-' ||
+                c == '_'
+                );
+        }
+
+        if (!needsQuoting)
+            return input;
+
+        StringBuilder builder = new StringBuilder(input.length() * 2);
+        builder.append("'");
+        builder.append(input, 0, --i);
+
+        while (i < input.length())
+        {
+            char c = input.charAt(i++);
+            if (c == '\'')
+            {
+                // There is no escape for a literal single quote, so we must leave the quotes
+                // and then escape the single quote. We test for the start/end of the string, so
+                // we can be less ugly in those cases.
+                if (i == 1)
+                    builder.insert(0, "\\").append("'");
+                else if (i == input.length())
+                    builder.append("'\\");
+                else
+                    builder.append("'\\''");
+            }
+            else
+                builder.append(c);
+        }
+        builder.append("'");
+
+        return builder.toString();
+    }
+
+    /**
+     * Add a simple argument to the command line, quoted if necessary.
      *
      * @param arg the simple argument to add
      */
@@ -91,49 +163,93 @@ public class CommandLineBuilder
     {
         if (arg != null)
         {
+            if (commandLine.length() > 0)
+                commandLine.append(separator);
             args.add(arg);
+            commandLine.append(shellQuoteIfNeeded(arg));
         }
     }
 
     /**
-     * Similar to {@link #addArg(String)} but concats both name + value with an "=" sign, quoting were needed, and excluding the "=" portion if the value is
+     * @deprecated use {@link #addArg(String, String)}
-     * undefined or empty.
+     */
-     *
+    @Deprecated
-     * <pre>
+    public void addEqualsArg(String name, String value)
-     *   addEqualsArg("-Dname", "value") = "-Dname=value"
+    {
-     *   addEqualsArg("-Djetty.home", "/opt/company inc/jetty (7)/") = "-Djetty.home=/opt/company\ inc/jetty\ (7)/"
+        addArg(name, value);
-     *   addEqualsArg("-Djenkins.workspace", "/opt/workspaces/jetty jdk7/") = "-Djenkins.workspace=/opt/workspaces/jetty\ jdk7/"
+    }
-     *   addEqualsArg("-Dstress", null) = "-Dstress"
+
-     *   addEqualsArg("-Dstress", "") = "-Dstress"
+    /**
-     * </pre>
+     * Add a "name=value" style argument to the command line with
-     *
+     * name and value quoted if necessary.
      * @param name the name
      * @param value the value
      */
-    public void addEqualsArg(String name, String value)
+    public void addArg(String name, String value)
     {
+        Objects.requireNonNull(name);
+
+        if (commandLine.length() > 0)
+            commandLine.append(separator);
+
         if ((value != null) && (value.length() > 0))
         {
             args.add(name + "=" + value);
+            commandLine.append(shellQuoteIfNeeded(name)).append('=').append(shellQuoteIfNeeded(value));
         }
         else
         {
             args.add(name);
+            commandLine.append(shellQuoteIfNeeded(name));
         }
     }
 
     /**
-     * Add a simple argument to the command line.
+     * @deprecated use {@link #addArg(String)}
-     * <p>
-     * Will <b>NOT</b> quote/escape arguments that have a space in them.
-     *
-     * @param arg the simple argument to add
      */
+    @Deprecated
     public void addRawArg(String arg)
     {
-        if (arg != null)
+        addArg(arg);
+    }
+
+    /**
+     * Adds a "-OPTION" style option to the command line with no quoting, for example `--help`.
+     * @param option the option
+     */
+    public void addOption(String option)
+    {
+        addOption(option, null, null);
+    }
+
+    /**
+     * Adds a "-OPTIONname=value" style option to the command line with
+     * name and value quoted if necessary, for example "-Dprop=value".
+     * @param option the option
+     * @param name the name
+     * @param value the value
+     */
+    public void addOption(String option, String name, String value)
+    {
+        Objects.requireNonNull(option);
+
+        if (commandLine.length() > 0)
+            commandLine.append(separator);
+
+        if (name == null || name.length() == 0)
         {
-            args.add(arg);
+            commandLine.append(option);
+            args.add(option);
+        }
+        else if ((value != null) && (value.length() > 0))
+        {
+            args.add(option + name + "=" + value);
+            commandLine.append(option).append(shellQuoteIfNeeded(name)).append('=').append(shellQuoteIfNeeded(value));
+        }
+        else
+        {
+            args.add(option + name);
+            commandLine.append(option).append(shellQuoteIfNeeded(name));
         }
     }
 
@@ -144,48 +260,35 @@ public class CommandLineBuilder
 
     @Override
     public String toString()
-    {
-        return toString(" ");
-    }
-
-    public String toString(String delim)
     {
         StringBuilder buf = new StringBuilder();
-
         for (String arg : args)
         {
             if (buf.length() > 0)
-            {
+                buf.append(' ');
-                buf.append(delim);
-            }
             buf.append(arg); // we assume escaping has occurred during addArg
         }
 
         return buf.toString();
     }
 
+    /**
+     * @deprecated use {@link #toCommandLine()}
+     */
+    @Deprecated
+    public String toQuotedString()
+    {
+        return toCommandLine();
+    }
+
     /**
      * A version of {@link #toString()} where every arg is evaluated for potential {@code '} (single-quote tick) wrapping.
      *
      * @return the toString but each arg that has spaces is surrounded by {@code '} (single-quote tick)
      */
-    public String toQuotedString()
+    public String toCommandLine()
     {
-        StringBuilder buf = new StringBuilder();
+        return commandLine.toString();
-
-        for (String arg : args)
-        {
-            if (buf.length() > 0)
-                buf.append(' ');
-            boolean needsQuotes = (arg.contains(" "));
-            if (needsQuotes)
-                buf.append("'");
-            buf.append(arg);
-            if (needsQuotes)
-                buf.append("'");
-        }
-
-        return buf.toString();
     }
 
     public void debug()

jetty-start/src/main/java/org/eclipse/jetty/start/Main.java

@@ -441,7 +441,7 @@ public class Main
         {
             CommandLineBuilder cmd = args.getMainArgs(args.getDryRunParts());
             cmd.debug();
-            System.out.println(cmd.toQuotedString());
+            System.out.println(cmd.toCommandLine());
         }
 
         if (args.isStopCommand())

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

@@ -208,6 +208,7 @@ public class StartArgs
     private boolean listConfig = false;
     private boolean version = false;
     private boolean dryRun = false;
+    private boolean multiLine = false;
     private final Set<String> dryRunParts = new HashSet<>();
     private boolean jpms = false;
     private boolean createStartD = false;
@@ -706,7 +707,7 @@ public class StartArgs
         if (parts.isEmpty())
             parts = ALL_PARTS;
 
-        CommandLineBuilder cmd = new CommandLineBuilder();
+        CommandLineBuilder cmd = new CommandLineBuilder(multiLine);
 
         // Special Stop/Shutdown properties
         ensureSystemPropertySet("STOP.PORT");
@@ -714,13 +715,13 @@ public class StartArgs
         ensureSystemPropertySet("STOP.WAIT");
 
         if (parts.contains("java"))
-            cmd.addRawArg(CommandLineBuilder.findJavaBin());
+            cmd.addArg(CommandLineBuilder.findJavaBin());
 
         if (parts.contains("opts"))
         {
-            cmd.addRawArg("-Djava.io.tmpdir=" + System.getProperty("java.io.tmpdir"));
+            cmd.addOption("-D", "java.io.tmpdir", System.getProperty("java.io.tmpdir"));
-            cmd.addRawArg("-Djetty.home=" + baseHome.getHome());
+            cmd.addOption("-D", "jetty.home", baseHome.getHome());
-            cmd.addRawArg("-Djetty.base=" + baseHome.getBase());
+            cmd.addOption("-D", "jetty.base", baseHome.getBase());
 
             for (String x : getJvmArgSources().keySet())
             {
@@ -731,11 +732,11 @@ public class StartArgs
                     String value = assign.length == 1 ? "" : assign[1];
 
                     Prop p = processSystemProperty(key, value, null);
-                    cmd.addRawArg("-D" + p.key + "=" + getProperties().expand(p.value));
+                    cmd.addOption("-D", p.key, properties.expand(p.value));
                 }
                 else
                 {
-                    cmd.addRawArg(getProperties().expand(x));
+                    cmd.addArg(getProperties().expand(x));
                 }
             }
 
@@ -743,7 +744,7 @@ public class StartArgs
             for (String propKey : systemPropertySource.keySet())
             {
                 String value = System.getProperty(propKey);
-                cmd.addEqualsArg("-D" + propKey, value);
+                cmd.addOption("-D", propKey, value);
             }
         }
 
@@ -756,60 +757,60 @@ public class StartArgs
                 List<File> files = dirsAndFiles.get(false);
                 if (files != null && !files.isEmpty())
                 {
-                    cmd.addRawArg("--module-path");
+                    cmd.addOption("--module-path");
                     String modules = files.stream()
                         .map(File::getAbsolutePath)
                         .collect(Collectors.joining(File.pathSeparator));
-                    cmd.addRawArg(modules);
+                    cmd.addArg(modules);
                 }
                 List<File> dirs = dirsAndFiles.get(true);
                 if (dirs != null && !dirs.isEmpty())
                 {
-                    cmd.addRawArg("--class-path");
+                    cmd.addOption("--class-path");
                     String directories = dirs.stream()
                         .map(File::getAbsolutePath)
                         .collect(Collectors.joining(File.pathSeparator));
-                    cmd.addRawArg(directories);
+                    cmd.addArg(directories);
                 }
 
                 if (!jmodAdds.isEmpty())
                 {
-                    cmd.addRawArg("--add-modules");
+                    cmd.addOption("--add-modules");
-                    cmd.addRawArg(String.join(",", jmodAdds));
+                    cmd.addArg(String.join(",", jmodAdds));
                 }
                 for (Map.Entry<String, Set<String>> entry : jmodPatch.entrySet())
                 {
-                    cmd.addRawArg("--patch-module");
+                    cmd.addOption("--patch-module");
-                    cmd.addRawArg(entry.getKey() + "=" + String.join(File.pathSeparator, entry.getValue()));
+                    cmd.addArg(entry.getKey(), String.join(File.pathSeparator, entry.getValue()));
                 }
                 for (Map.Entry<String, Set<String>> entry : jmodOpens.entrySet())
                 {
-                    cmd.addRawArg("--add-opens");
+                    cmd.addOption("--add-opens");
-                    cmd.addRawArg(entry.getKey() + "=" + String.join(",", entry.getValue()));
+                    cmd.addArg(entry.getKey(), String.join(",", entry.getValue()));
                 }
                 for (Map.Entry<String, Set<String>> entry : jmodExports.entrySet())
                 {
-                    cmd.addRawArg("--add-exports");
+                    cmd.addOption("--add-exports");
-                    cmd.addRawArg(entry.getKey() + "=" + String.join(",", entry.getValue()));
+                    cmd.addArg(entry.getKey(), String.join(",", entry.getValue()));
                 }
                 for (Map.Entry<String, Set<String>> entry : jmodReads.entrySet())
                 {
-                    cmd.addRawArg("--add-reads");
+                    cmd.addOption("--add-reads");
-                    cmd.addRawArg(entry.getKey() + "=" + String.join(",", entry.getValue()));
+                    cmd.addArg(entry.getKey(), String.join(",", entry.getValue()));
                 }
             }
             else
             {
-                cmd.addRawArg("--class-path");
+                cmd.addOption("--class-path");
-                cmd.addRawArg(classpath.toString());
+                cmd.addArg(classpath.toString());
             }
         }
 
         if (parts.contains("main"))
         {
             if (isJPMS())
-                cmd.addRawArg("--module");
+                cmd.addOption("--module");
-            cmd.addRawArg(getMainClassname());
+            cmd.addArg(getMainClassname());
         }
 
         // pass properties as args or as a file
@@ -819,7 +820,8 @@ public class StartArgs
             {
                 for (Prop p : properties)
                 {
-                    cmd.addRawArg(CommandLineBuilder.quote(p.key) + "=" + CommandLineBuilder.quote(properties.expand(p.value)));
+                    if (!p.key.startsWith("java.version."))
+                        cmd.addArg(p.key, properties.expand(p.value));
                 }
             }
             else if (properties.size() > 0)
@@ -837,17 +839,17 @@ public class StartArgs
                 {
                     properties.store(out, "start.jar properties");
                 }
-                cmd.addRawArg(propPath.toAbsolutePath().toString());
+                cmd.addArg(propPath.toAbsolutePath().toString());
             }
 
             for (Path xml : xmls)
             {
-                cmd.addRawArg(xml.toAbsolutePath().toString());
+                cmd.addArg(xml.toAbsolutePath().toString());
             }
 
             for (Path propertyFile : propertyFiles)
             {
-                cmd.addRawArg(propertyFile.toAbsolutePath().toString());
+                cmd.addArg(propertyFile.toAbsolutePath().toString());
             }
         }
 
@@ -1225,6 +1227,12 @@ public class StartArgs
             int colon = arg.indexOf('=');
             for (String part : arg.substring(colon + 1).split(","))
             {
+                if ("multiline".equalsIgnoreCase(part))
+                {
+                    multiLine = true;
+                    continue;
+                }
+
                 if (!ALL_PARTS.contains(part))
                     throw new UsageException(UsageException.ERR_BAD_ARG, "Unrecognized --dry-run=\"%s\" in %s", part, source);
 

jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt

@@ -60,12 +60,13 @@ Report Commands:
 
   --dry-run
                    Prints the command line that start.jar generates,
-                   then exits.
+                   in a format usable by a POSIX compliant shell, then exits.
                    This may be used to generate command lines into scripts:
                      $ java -jar start.jar --dry-run > jetty.sh
 
   --dry-run=<part>(,<part>)*
-                   Prints specific parts of the command line. The parts are:
+                   Prints specific parts of the command line in a format usable by
+                   a POSIX compliant shell. The parts are:
                      o  "java" - the JVM to run
                      o  "opts" - the JVM options (e.g. -D, -X and -XX flags)
                      o  "path" - the JVM class-path and/or the JPMS module-path

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java

@@ -13,7 +13,12 @@
 
 package org.eclipse.jetty.start;
 
+import java.util.stream.Stream;
+
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
@@ -23,43 +28,90 @@ public class CommandLineBuilderTest
     @Test
     public void testSimpleCommandline()
     {
-        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        CommandLineBuilder cmd = new CommandLineBuilder();
-        cmd.addEqualsArg("-Djava.io.tmpdir", "/home/java/temp dir/");
+        cmd.addArg("java");
+        cmd.addArg("-Djava.io.tmpdir", "/home/java/temp dir/");
         cmd.addArg("--version");
-        assertThat(cmd.toQuotedString(), is("java '-Djava.io.tmpdir=/home/java/temp dir/' --version"));
+        assertThat(cmd.toCommandLine(), is("java -Djava.io.tmpdir='/home/java/temp dir/' --version"));
     }
 
     @Test
     public void testSimpleHomeNoSpace()
     {
-        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        CommandLineBuilder cmd = new CommandLineBuilder();
-        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        cmd.addArg("java");
-        assertThat(cmd.toQuotedString(), is("java -Djetty.home=/opt/jetty"));
+        cmd.addArg("-Djetty.home", "/opt/jetty");
+        assertThat(cmd.toCommandLine(), is("java -Djetty.home=/opt/jetty"));
     }
 
     @Test
     public void testSimpleHomeWithSpace()
     {
-        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        CommandLineBuilder cmd = new CommandLineBuilder();
-        cmd.addEqualsArg("-Djetty.home", "/opt/jetty 10/home");
+        cmd.addArg("java");
-        assertThat(cmd.toQuotedString(), is("java '-Djetty.home=/opt/jetty 10/home'"));
+        cmd.addArg("-Djetty.home", "/opt/jetty 10/home");
+        assertThat(cmd.toCommandLine(), is("java -Djetty.home='/opt/jetty 10/home'"));
     }
 
     @Test
     public void testEscapedFormattingString()
     {
-        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        CommandLineBuilder cmd = new CommandLineBuilder();
-        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        cmd.addArg("java");
-        cmd.addEqualsArg("jetty.requestlog.formatter", "%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"");
+        cmd.addArg("-Djetty.home", "/opt/jetty");
-        assertThat(cmd.toQuotedString(), is("java -Djetty.home=/opt/jetty 'jetty.requestlog.formatter=%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"'"));
+        cmd.addArg("jetty.requestlog.formatter", "%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"");
+        assertThat(cmd.toCommandLine(), is("java -Djetty.home=/opt/jetty jetty.requestlog.formatter='%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"'"));
     }
 
     @Test
     public void testEscapeUnicode()
     {
-        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        CommandLineBuilder cmd = new CommandLineBuilder();
-        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        cmd.addArg("java");
-        cmd.addEqualsArg("monetary.symbol", "€");
+        cmd.addArg("-Djetty.home", "/opt/jetty");
-        assertThat(cmd.toQuotedString(), is("java -Djetty.home=/opt/jetty monetary.symbol=€"));
+        cmd.addArg("monetary.symbol", "€");
+        assertThat(cmd.toCommandLine(), is("java -Djetty.home=/opt/jetty monetary.symbol='€'"));
+    }
+
+    public static Stream<Arguments> shellQuoting()
+    {
+        return Stream.of(
+            Arguments.of(null, null),
+            Arguments.of("", "''"),
+            Arguments.of("Hello", "Hello"),
+            Arguments.of("Hell0", "Hell0"),
+            Arguments.of("Hello$World", "'Hello$World'"),
+            Arguments.of("Hello\\World", "'Hello\\World'"),
+            Arguments.of("Hello`World", "'Hello`World'"),
+            Arguments.of("'Hello World'", "\\''Hello World'\\'"),
+            Arguments.of("\"Hello World\"", "'\"Hello World\"'"),
+            Arguments.of("H-llo_world", "H-llo_world"),
+            Arguments.of("H:llo/world", "H:llo/world"),
+            Arguments.of("Hello World", "'Hello World'"),
+            Arguments.of("foo\\bar", "'foo\\bar'"),
+            Arguments.of("foo'bar", "'foo'\\''bar'"),
+            Arguments.of("some 'internal' quoting", "'some '\\''internal'\\'' quoting'"),
+            Arguments.of("monetary.symbol=€", "'monetary.symbol=€'")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("shellQuoting")
+    public void testShellQuoting(String string, String expected)
+    {
+        assertThat(CommandLineBuilder.shellQuoteIfNeeded(string), is(expected));
+    }
+
+    @Test
+    public void testMultiLine()
+    {
+        CommandLineBuilder cmd = new CommandLineBuilder(true);
+        cmd.addArg("java");
+        cmd.addArg("-Djetty.home", "/opt/jetty");
+        cmd.addArg("monetary.symbol", "€");
+        assertThat(cmd.toCommandLine(),
+            is("java \\" + System.lineSeparator() +
+                "  -Djetty.home=/opt/jetty \\" + System.lineSeparator() +
+                "  monetary.symbol='€'"));
     }
 }

jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java

@@ -174,7 +174,7 @@ public class MainTest
         assertThat("jetty.base", baseHome.getBase(), is(homePath.toString()));
 
         CommandLineBuilder commandLineBuilder = args.getMainArgs(StartArgs.ALL_PARTS);
-        String commandLine = commandLineBuilder.toString("\n");
+        String commandLine = commandLineBuilder.toString();
         String expectedExpansion = String.format("-Xloggc:%s/logs/gc-%s.log",
             baseHome.getBase(), System.getProperty("java.version")
         );