Issue #9309 - Better jetty.sh integration for start.jar with eye on supporting odd properties

.github/workflows/codeql-analysis.yml

@@ -24,7 +24,8 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        language: [ 'java']
+        languages:
+          - java
         # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ]
         # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support
 
@@ -62,7 +63,7 @@ jobs:
       - name: Initialize CodeQL
         uses: github/codeql-action/init@v2
         with:
-          languages: ${{ matrix.language }}
+          languages: ${{ matrix.languages }}
           # If you wish to specify custom queries, you can do so here or in a config file.
           # By default, queries listed here will override any specified in a config file.
           # Prefix the list here with "+" to use these queries and those in the config file.
@@ -71,7 +72,7 @@ jobs:
           # queries: security-extended,security-and-quality
 
       - name: Set up Maven
-        uses: stCarolas/setup-maven@v4
+        uses: stCarolas/setup-maven@v4.5
         with:
           maven-version: 3.8.6   
 

jetty-home/src/main/resources/bin/jetty.sh

@@ -170,7 +170,8 @@ dumpEnv()
     echo "JETTY_START_LOG       =  $JETTY_START_LOG"
     echo "JETTY_STATE           =  $JETTY_STATE"
     echo "JETTY_START_TIMEOUT   =  $JETTY_START_TIMEOUT"
-    echo "RUN_CMD               =  ${RUN_CMD[*]}"
+    echo "JETTY_SYS_PROPS       =  $JETTY_SYS_PROPS"
+    echo "RUN_ARGS              =  ${RUN_ARGS[*]}"
 }
 
 
@@ -414,9 +415,6 @@ TMPDIR="`cygpath -w $TMPDIR`"
 ;;
 esac
 
-BASE_JETTY_SYS_PROPS=$(echo -ne "-Djetty.home=$JETTY_HOME" "-Djetty.base=$JETTY_BASE" "-Djava.io.tmpdir=$TMPDIR")
-JETTY_SYS_PROPS=(${JETTY_SYS_PROPS[*]} $BASE_JETTY_SYS_PROPS)
-
 #####################################################
 # This is how the Jetty server will be started
 #####################################################
@@ -434,8 +432,9 @@ case "`uname`" in
 CYGWIN*) JETTY_START="`cygpath -w $JETTY_START`";;
 esac
 
-RUN_ARGS=$("$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]})
-RUN_CMD=("$JAVA" $JETTY_SYS_PROPS ${RUN_ARGS[@]})
+# Collect the dry-run (of opts,path,main,args) from the jetty.base configuration
+JETTY_DRY_RUN=$("$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]})
+RUN_ARGS=($JETTY_SYS_PROPS ${JETTY_DRY_RUN[@]})
 
 #####################################################
 # Comment these out after you're happy with what
@@ -466,13 +465,14 @@ case "$ACTION" in
         CH_USER="--chuid $JETTY_USER"
       fi
 
-      start-stop-daemon --start $CH_USER \
+      echo ${RUN_ARGS[@]} start-log-file="$JETTY_START_LOG" | xargs start-stop-daemon \
+       --start $CH_USER \
        --pidfile "$JETTY_PID" \
        --chdir "$JETTY_BASE" \
        --background \
        --make-pidfile \
        --startas "$JAVA" \
-       -- ${RUN_ARGS[@]} start-log-file="$JETTY_START_LOG"
+       --
 
     else
 
@@ -495,11 +495,11 @@ case "$ACTION" in
         # FIXME: Broken solution: wordsplitting, pathname expansion, arbitrary command execution, etc.
         su - "$JETTY_USER" $SU_SHELL -c "
           cd \"$JETTY_BASE\"
-          exec ${RUN_CMD[*]} start-log-file=\"$JETTY_START_LOG\" > /dev/null &
+          echo ${RUN_ARGS[*]} start-log-file=\"$JETTY_START_LOG\" | xargs ${JAVA} > /dev/null &
           disown \$!
           echo \$! > \"$JETTY_PID\""
       else
-        "${RUN_CMD[@]}" > /dev/null &
+        echo ${RUN_ARGS[*]} | xargs ${JAVA} > /dev/null &
         disown $!
         echo $! > "$JETTY_PID"
       fi
@@ -584,7 +584,7 @@ case "$ACTION" in
     # Under control of daemontools supervise monitor which
     # handles restarts and shutdowns via the svc program.
     #
-    exec "${RUN_CMD[@]}"
+    echo ${RUN_ARGS[*]} | xargs ${JAVA} > /dev/null &
 
     ;;
 
@@ -597,7 +597,7 @@ case "$ACTION" in
       exit 1
     fi
 
-    exec "${RUN_CMD[@]}"
+    echo ${RUN_ARGS[*]} | xargs ${JAVA} > /dev/null &
     ;;
 
   check|status)

jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java

@@ -571,14 +571,14 @@ else if (m.group("LITERAL") != null)
                 }
                 else
                 {
-                    throw new IllegalStateException("formatString parsing error");
+                    throw new IllegalStateException("formatString parsing error: " + formatString);
                 }
 
                 remaining = m.group("REMAINING");
             }
             else
             {
-                throw new IllegalArgumentException("Invalid format string");
+                throw new IllegalArgumentException("Invalid format string: " + formatString);
             }
         }
 

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

@@ -59,34 +59,12 @@ public static String findJavaBin()
      *
      * @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)
     {
-        boolean needsQuoting = (arg.indexOf(' ') >= 0) || (arg.indexOf('"') >= 0);
-        if (!needsQuoting)
-        {
-            return arg;
-        }
-        StringBuilder buf = new StringBuilder();
-        // buf.append('"');
-        boolean escaped = false;
-        boolean quoted = false;
-        for (char c : arg.toCharArray())
-        {
-            if (!quoted && !escaped && ((c == '"') || (c == ' ')))
-            {
-                buf.append("\\");
-            }
-            // don't quote text in single quotes
-            if (!escaped && (c == '\''))
-            {
-                quoted = !quoted;
-            }
-            escaped = (c == '\\');
-            buf.append(c);
-        }
-        // buf.append('"');
-        return buf.toString();
+        return "'" + arg + "'";
     }
 
     private List<String> args;
@@ -113,7 +91,7 @@ public void addArg(String arg)
     {
         if (arg != null)
         {
-            args.add(quote(arg));
+            args.add(arg);
         }
     }
 
@@ -136,11 +114,11 @@ public void addEqualsArg(String name, String value)
     {
         if ((value != null) && (value.length() > 0))
         {
-            args.add(quote(name + "=" + value));
+            args.add(name + "=" + value);
         }
         else
         {
-            args.add(quote(name));
+            args.add(name);
         }
     }
 
@@ -180,7 +158,31 @@ public String toString(String delim)
             {
                 buf.append(delim);
             }
-            buf.append(quote(arg));
+            buf.append(arg); // we assume escaping has occurred during addArg
+        }
+
+        return buf.toString();
+    }
+
+    /**
+     * 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()
+    {
+        StringBuilder buf = new StringBuilder();
+
+        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();

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

@@ -440,7 +440,8 @@ public void start(StartArgs args) throws IOException, InterruptedException
         if (args.isDryRun())
         {
             CommandLineBuilder cmd = args.getMainArgs(args.getDryRunParts());
-            System.out.println(cmd.toString(StartLog.isDebugEnabled() ? " \\\n" : " "));
+            cmd.debug();
+            System.out.println(cmd.toQuotedString());
         }
 
         if (args.isStopCommand())

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

@@ -13,62 +13,53 @@
 
 package org.eclipse.jetty.start;
 
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 
 public class CommandLineBuilderTest
 {
-    private CommandLineBuilder cmd = new CommandLineBuilder("java");
-
-    @BeforeEach
-    public void setUp()
-    {
-        cmd.addEqualsArg("-Djava.io.tmpdir", "/home/java/temp dir/");
-        cmd.addArg("--version");
-    }
-
     @Test
     public void testSimpleCommandline()
     {
-        assertThat(cmd.toString(), is("java -Djava.io.tmpdir=/home/java/temp\\ dir/ --version"));
-    }
-
-    @Test
-    public void testQuotingSimple()
-    {
-        assertQuoting("/opt/jetty", "/opt/jetty");
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djava.io.tmpdir", "/home/java/temp dir/");
+        cmd.addArg("--version");
+        assertThat(cmd.toQuotedString(), is("java '-Djava.io.tmpdir=/home/java/temp dir/' --version"));
     }
 
     @Test
-    public void testQuotingSpaceInPath()
+    public void testSimpleHomeNoSpace()
     {
-        assertQuoting("/opt/jetty 7/home", "/opt/jetty\\ 7/home");
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        assertThat(cmd.toQuotedString(), is("java -Djetty.home=/opt/jetty"));
     }
 
     @Test
-    public void testQuotingSpaceAndQuotesInPath()
+    public void testSimpleHomeWithSpace()
     {
-        assertQuoting("/opt/jetty 7 \"special\"/home", "/opt/jetty\\ 7\\ \\\"special\\\"/home");
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djetty.home", "/opt/jetty 10/home");
+        assertThat(cmd.toQuotedString(), is("java '-Djetty.home=/opt/jetty 10/home'"));
     }
 
     @Test
-    public void testToStringIsQuotedEvenIfArgsAreNotQuotedForProcessBuilder()
+    public void testEscapedFormattingString()
     {
-        System.out.println(cmd.toString());
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        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\"");
+        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\"'"));
     }
 
     @Test
-    public void testQuoteQuotationMarks()
-    {
-        assertQuoting("-XX:OnOutOfMemoryError='kill -9 %p'", "-XX:OnOutOfMemoryError='kill -9 %p'");
-    }
-
-    private void assertQuoting(String raw, String expected)
+    public void testEscapeUnicode()
     {
-        String actual = CommandLineBuilder.quote(raw);
-        assertThat("Quoted version of [" + raw + "]", actual, is(expected));
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djetty.home", "/opt/jetty");
+        cmd.addEqualsArg("monetary.symbol", "€");
+        assertThat(cmd.toQuotedString(), is("java -Djetty.home=/opt/jetty monetary.symbol=€"));
     }
 }

tests/test-sessions/test-mongodb-sessions/src/test/java/org/eclipse/jetty/nosql/mongodb/AttributeNameTest.java

@@ -31,7 +31,10 @@
 import org.eclipse.jetty.util.NanoTime;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledForJreRange;
+import org.junit.jupiter.api.condition.JRE;
 import org.testcontainers.junit.jupiter.Testcontainers;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -43,6 +46,7 @@
  * See bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=444595
  */
 @Testcontainers(disabledWithoutDocker = true)
+@Tag("flaky")
 public class AttributeNameTest
 {
     @BeforeAll