do not automatically quote system properties containing spaces #9999
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
There was a problem hiding this comment.
I am a bit concerned that since we added the quoting of start args, we are finding issues bit by bit. Is reverting this going to cause new issues or return to an existing issue.
@joakime can you confirm that this change still passes the tests you created for why the quoting was changed in the first place?
|
@joakime nudge? |
| @@ -441,7 +441,7 @@ public void start(StartArgs args) throws IOException, InterruptedException | |||
| { | |||
| CommandLineBuilder cmd = args.getMainArgs(args.getDryRunParts()); | |||
| cmd.debug(); | |||
| System.out.println(cmd.toQuotedString()); | |||
| System.out.println(cmd); | |||
There was a problem hiding this comment.
This breaks jetty.sh usage.
See #9309
I'll try to write a DistributionTest (in this branch) that uses jetty.sh (test would be limited to only executing on Linux env)
There was a problem hiding this comment.
Hmm, this is tricky, as it's a forked Exec (to bash + jetty.sh) that execs a JVM.
Getting the pid to stop the running JVM isn't that straight forward. (nor does jetty.sh stop work on this forked/execed JVM)
I'm not going to continue with this testcase, I'm abandoning this effort.
Here's the scenario, in the form of a jetty base, that would cause the issue in #9309
[requestlog-custom-base]$ tree -F
./
├── logs/
├── resources/
│ └── jetty-logging.properties
├── start.d/
│ ├── custom-system-props.ini
│ ├── deploy.ini
│ ├── http.ini
│ └── requestlog.ini
└── webapps/
4 directories, 5 files
[requestlog-custom-base]$ cat start.d/deploy.ini
--module=deploy
[requestlog-custom-base]$ cat start.d/http.ini
--module=http
[requestlog-custom-base]$ cat start.d/requestlog.ini
--module=requestlog
jetty.requestlog.formatString=%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t "%r" %s %O "%{Referer}i" "%{User-Agent}i"
[requestlog-custom-base]$ cat start.d/custom-system-props.ini
--exec
-Dprop="foo bar"
[requestlog-custom-base]$
joakime
dismissed
their stale review
Command line testing with jetty.sh shows this PR seems to be working
|
Testing this branch Using the previously mentioned The output is as follows ... Which shows that the 2 relevant properties to this PR as ...
|
|
Digging into this some more, as I was curious how this would impact the behavior of normal vs forked runs too. So, I used the args-dump tool from https://github.com/joakime/args-dump/releases/tag/v1.1 [requestlog-custom-base]$ cat start.d/args-dump.ini
--lib=/home/joakim/java/args-dump-1.1.jar
main.class=net.erdfelt.ArgsDumpWith system property and [requestlog-custom-base]$ ls -la start.d/
total 28
-rw-rw-r-- 1 joakim joakim 74 Jul 5 10:07 args-dump.ini
-rw-rw-r-- 1 joakim joakim 24 Jul 5 09:14 custom-system-props.ini
-rw-rw-r-- 1 joakim joakim 16 Jul 5 09:19 deploy.ini
-rw-rw-r-- 1 joakim joakim 14 Jul 5 09:19 http.ini
-rw-rw-r-- 1 joakim joakim 142 Jul 5 09:10 requestlog.ini
[requestlog-custom-base]$ java -jar ~/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/start.jar
WARN : Forking second JVM due to forking module(s): []. Use --dry-run to generate the command line to avoid forking.
System Property[jetty.base]: [/home/joakim/code/jetty/distros/bases/requestlog-custom-base]
System Property[jetty.home]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home]
System Property[prop]: ["foo bar"]
args.length=8
args[0]: [/tmp/start_16907308263693125229.properties]
> [java.version]: [17.0.6]
> [java.version.major]: [17]
> [java.version.micro]: [6]
> [java.version.minor]: [0]
> [java.version.platform]: [17]
> [jetty.base]: [/home/joakim/code/jetty/distros/bases/requestlog-custom-base]
> [jetty.base.uri]: [file:///home/joakim/code/jetty/distros/bases/requestlog-custom-base]
> [jetty.home]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home]
> [jetty.home.uri]: [file:///home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home]
> [jetty.requestlog.formatString]: [%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t "%r" %s %O "%{Referer}i" "%{User-Agent}i"]
> [jetty.webapp.addServerClasses]: [org.eclipse.jetty.logging.,file:///home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/lib/logging/,org.slf4j.]
> [main.class]: [net.erdfelt.ArgsDump]
> [prop]: ["foo bar"]
> [runtime.feature.alpn]: [true]
> [slf4j.version]: [2.0.5]
args[1]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-bytebufferpool.xml]
args[2]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-threadpool.xml]
args[3]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty.xml]
args[4]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-webapp.xml]
args[5]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-deploy.xml]
args[6]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-http.xml]
args[7]: [/home/joakim/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/etc/jetty-requestlog.xml]
Without system property (and no forking) ... Aside: I find it interesting that we are not consistent with our System Properties between normal vs forked JVMs (but that's a new issue, not related this issue, opened as #10065). The System Property shows up as key $ java -Dprop="foo bar" -jar ~/java/args-dump-1.1.jar xxx yyy="yup" zzz
System Property[prop]: [foo bar]
args.length=3
args[0]: [xxx]
args[1]: [yyy=yup]
args[2]: [zzz]That means our own internal handling of a system property from a |
|
If we use a non-quoted system property in a Example: [requestlog-custom-base]$ cat start.d/custom-system-props.ini
--exec
-Dprop=foo bar
[requestlog-custom-base]$ java -jar ~/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/start.jar --dry-run
...The output shows ... BUT, if we use Which is now a correctly formatted system property for |
| @@ -441,7 +441,7 @@ public void start(StartArgs args) throws IOException, InterruptedException | |||
| { | |||
| CommandLineBuilder cmd = args.getMainArgs(args.getDryRunParts()); | |||
| cmd.debug(); | |||
| System.out.println(cmd.toQuotedString()); | |||
| System.out.println(cmd); | |||
There was a problem hiding this comment.
This needs to be fixed, but I suspect in a grander scale than this PR currently has done.
The quoting is here to compensate for (system property) arguments specified in start.d/*.ini files.
Reminder, the quoting -Dprop="foo bar" is not what is used in a start.d/*.ini file, as that double quote only exists for shell usages.
As each line in an INI is a whole command line argument, the normal start.jar and even forked (usage of --exec) will operate properly (thanks be to the List<String> separation of arguments for either direct invocation of the main class, or the forked JVM use of java.lang.ProcessBuilder(List<String>))
Removing it breaks that kind of usage.
There was a problem hiding this comment.
@joakime Sorry but I'm not follow the pronouns... what "it" needs to be removed? what "this" needs to be fixed?
Can you and @lachlan-roberts get together and come up with a grander scale fix for this. Good point that our ini files are already expanded, so that any quotes in them are literal quotes not shell quotes. We need to be very clear about all our spaces if they are expanded or not... and the bottom line is that the output of --dry-run needs to be able to be executed by a shell or by a shell script.
There was a problem hiding this comment.
This is bigger than --dry-run alone ...
Use Cases (A): --dry-run output use/consumption
- Executed by unix shell directly (eg:
exec $ARGS) - Streamed into executable / pipe in unix via args (eg:
xargs) - Used as arguments in java daemon
- Used as arguments in linux systemd startup metadata
- Used as arguments in windows daemon
- Used in
jetty.sh
Use Cases (B): Quoting of arguments by start.jar
- From command line
- From start.d/*.ini with quotes
- From start.d/*.ini without quotes
- From module declaration with quotes
- From module declarattion without quotes
Use Cases (C): How the arguments are consumed
- The output of
--dry-run - Direct
main.classinvoke ofmain(String[] args) - Use in
java.lang.ProcessBuilder(List<String> args)
The change that lachlan did in this PR ...
- Addressed use cases A1, A2, A6, B1, B2, B4, C1
- But Broke use cases A3, A4, A5, B3+C2, B3+C3, B5+C2, B5+C3
There was a problem hiding this comment.
This is bigger than
--dry-runalone ...
Agreed. Rather than arbitrarily going through start.jar adding or removing quotes, we need to step back and be very rigorous in understanding what spaces we have and if they are quoted or unquoted.
Use Cases (A): --dry-run output use/consumption
- Executed by unix shell directly (eg:
exec $ARGS)
Obviously this is a quoted valued, so that it can be cut and pasted on the command line and executed.
- Streamed into executable / pipe in unix via args (eg:
xargs)
ditto
- Used as arguments in java daemon
ditto
- Used as arguments in linux systemd startup metadata
ditto
- Used as arguments in windows daemon
Interesting... are windows command line quoting conventions the same as unix?
- Used in
jetty.sh
This is also a quoted value.
I.e. The output of --dry-run should be quoted as necessary in all circumstances so that it can be correctly interpreted by a shell.
Use Cases (B): Quoting of arguments by start.jar
I think this is the more interesting area and probably where our problems lie:
- From command line
The command line is expanded by the shell, thus the arguments that we receive are unquoted.
- From start.d/*.ini with quotes
Our ini files are line separated, so they do not need to be quoted just for separation and preservation of spaces. So is this an unquoted space?
However, we do expand ${name} variables in our ini-files, so do we need an escape if an argument needs to have a value that actually contains ${? If we need an escape, then this perhaps a quoted space? Or some hybrid where we assume every line is preceded and terminated with a "?
- From start.d/*.ini without quotes
As above.
- From module declaration with quotes
Lines from mod files are copied directly to be lines in ini files, so whatever space ini files are in, then mod file lines are the same.
- From module declarattion without quotes
As above.
So I do not think our issue is with the output of --dry-run, as that simply must be correctly quoted so that a shell understands the argument separators and values.
I think our issue is with our inputs and determining if they are or are not quoted.
Specifically for this particular issue:
- we have an input of
-Dprop="\"foo bar\""given on the command line. this is a quoted value - this is after the
-jarargument so it is passed to the main method as an argument with valueprop="foo bar", which is unquoted - The output of the
--dry-runrequotes this (as it should) to be'-Dprop="foo bar"'. I'm not seeing the problem with this? Perhaps it would be better to quote name and value separately to-Dprop="foo bar"'`, but the value should be the same..... ok so I'm not understanding part of the original problem..... I'll experiment some more before commenting more.
|
@joakime @lachlan-roberts I've done a bit of experimentation and I can definitely see that start.jar is quoting things: incorrectly; too often; and sometimes not when needed. In an empty directory I ran the command: > java -jar ../start.jar -Dprop1="foo bar" -Dprop2="foo'bar" --dry-runWhich produced the output: which I'll line break here for clarity (it would be good to have a The problems I can see with this are as follows:
I think the output should be (assuming we need both system and jetty properties for Thus I think we need a utility @joakime do you want to take over this PR(or issue) and fix this? |
|
Update. I don't think we should be using single quotes at all. From "man sh" I see: So this indicates that I'm wrong in my previous comment when I suggested we should have Thus I think output should be (see new prop3): |
|
Note also from the bash manual: So as bash is often used as the shell, we should treat all of these characters as needing to be quoted as well. |
|
@joakime I think the quote method should be something like: public static String quoteIfNeededForShell(String input)
{
if (input.codePoints().noneMatch(StringUtil::isQuotingNeededForCodePoint))
return input; // No quoting necessary
StringBuilder output = new StringBuilder(input.length() + 16);
output.append('"'); // Start with a double quote
input.codePoints().forEach(c ->
{
switch (c)
{
case '"', '\\', '`', '$' -> output.append('\\').appendCodePoint(c);
default -> output.appendCodePoint(c);
}
});
output.append('"');
return output.toString();
}
public static boolean isQuotingNeededForCodePoint(int codePoint)
{
int type = Character.getType(codePoint);
return switch (type)
{
case Character.SPACE_SEPARATOR,
Character.LINE_SEPARATOR,
Character.PARAGRAPH_SEPARATOR,
Character.CONTROL,
Character.FORMAT,
Character.PRIVATE_USE,
Character.SURROGATE,
Character.OTHER_SYMBOL -> true;
default -> codePoint == '"' ||
codePoint == '\\' ||
codePoint == '$' ||
codePoint == '`' ||
codePoint == '|' ||
codePoint == '&' ||
codePoint == ';' ||
codePoint == '(' ||
codePoint == ')' ||
codePoint == '<' ||
codePoint == '>' ||
codePoint == '!' ||
codePoint == '*';
};
} |
|
@joakime, you have a few other tasks, so I'll start work on this one. |
See Issue jetty/jetty.docker#148
Currently this command
Quotes the system property like this in the generated command
'-Dprop="foo bar"'.This means it is impossible to use spaces in properties.
Because of this I have removed the use of
CommandLineBuildertoQuotedString()and useCommandLineBuilder.toString()instead. And if you want the args quoted it will have to be done explicitly.