Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not automatically quote system properties containing spaces #9999

Closed
wants to merge 2 commits into from
Closed

do not automatically quote system properties containing spaces #9999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84ee447
do not automatically quote system properties containing spaces
lachlan-roberts Jun 30, 2023
0cdbb58
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
lachlan-roberts Jul 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jetty-start/src/main/java/org/eclipse/jetty/start/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]$

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bigger than --dry-run alone ...

Use Cases (A): --dry-run output use/consumption

  1. Executed by unix shell directly (eg: exec $ARGS)
  2. Streamed into executable / pipe in unix via args (eg: xargs)
  3. Used as arguments in java daemon
  4. Used as arguments in linux systemd startup metadata
  5. Used as arguments in windows daemon
  6. Used in jetty.sh

Use Cases (B): Quoting of arguments by start.jar

  1. From command line
  2. From start.d/*.ini with quotes
  3. From start.d/*.ini without quotes
  4. From module declaration with quotes
  5. From module declarattion without quotes

Use Cases (C): How the arguments are consumed

  1. The output of --dry-run
  2. Direct main.class invoke of main(String[] args)
  3. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime @lachlan-roberts

This is bigger than --dry-run alone ...

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

  1. 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.

  1. Streamed into executable / pipe in unix via args (eg: xargs)

ditto

  1. Used as arguments in java daemon

ditto

  1. Used as arguments in linux systemd startup metadata

ditto

  1. Used as arguments in windows daemon

Interesting... are windows command line quoting conventions the same as unix?

  1. 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:

  1. From command line

The command line is expanded by the shell, thus the arguments that we receive are unquoted.

  1. 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 "?

  1. From start.d/*.ini without quotes

As above.

  1. 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.

  1. 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 -jar argument so it is passed to the main method as an argument with value prop="foo bar", which is unquoted
  • The output of the --dry-run requotes 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.

}

if (args.isStopCommand())
Expand Down
8 changes: 6 additions & 2 deletions ...st-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,13 +1308,17 @@ public void testDryRunProperties() throws Exception
assertTrue(run1.awaitFor(10, TimeUnit.SECONDS));
assertEquals(0, run1.getExitValue());

String[] args2 = {"--dry-run"};
String systemProp = "-Dtest=\"foo bar\"";
String[] args2 = {systemProp, "--dry-run"};
try (JettyHomeTester.Run run2 = distribution.start(args2))
{
run2.awaitFor(5, TimeUnit.SECONDS);
Queue<String> logs = run2.getLogs();
assertThat(logs.size(), equalTo(1));
assertThat(logs.poll(), not(containsString("${jetty.home.uri}")));
String log = logs.poll();
assertThat(log, not(containsString("${jetty.home.uri}")));
assertThat(log, not(containsString("'" + systemProp)));
assertThat(log, containsString(systemProp));
}
}
}
Expand Down