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.

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

Quoting for start arguments #10099

Merged
merged 13 commits into from
Jul 14, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -53,31 +53,82 @@ public static String findJavaBin()
return "java";
}

/**
* 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
* won't be escaped.
*
* @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)
private final StringBuilder commandLine = new StringBuilder();
private final List<String> args = new ArrayList<>();
private final String separator;

public CommandLineBuilder()
{
return "'" + arg + "'";
this(false);
}

private List<String> args;

public CommandLineBuilder()
public CommandLineBuilder(boolean multiline)
{
args = new ArrayList<String>();
separator = multiline ? (" \\" + System.lineSeparator() + " ") : " ";
}

public CommandLineBuilder(String bin)
/**
* Quote a string suitable for use with a command line shell using double quotes.
* <p>This method applies doubles quoting as described for the unix {@code sh} commands:
* Enclosing characters within double quotes preserves the literal meaning of all characters except
* dollarsign ($), backquote (`), and backslash (\).
* The backslash inside double quotes is historically weird, and serves
* to quote only the following characters: {@code $ ` " \ newline}.
* Otherwise, it remains literal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Quote a string suitable for use with a command line shell using double quotes.
* <p>This method applies doubles quoting as described for the unix {@code sh} commands:
* Enclosing characters within double quotes preserves the literal meaning of all characters except
* dollarsign ($), backquote (`), and backslash (\).
* The backslash inside double quotes is historically weird, and serves
* to quote only the following characters: {@code $ ` " \ newline}.
* Otherwise, it remains literal.
* Quote a string suitable for use with a command line shell using strong quotes {@code '}.
* <p>This method applies strong quoting as described for the unix {@code sh} commands:
* Enclosing characters within strong quotes preserves the literal meaning of all characters.

We don't want to use double quotes with sh / bash / zsh, we want to use strong quotes ', as that will not interpret the contents between the quotes.

Examples:

sh on ubuntu

[joakim@hyperion ~]$ which sh
/usr/bin/sh
[joakim@hyperion ~]$ ls -la /usr/bin/sh
lrwxrwxrwx 1 root root 4 Mar 23  2022 /usr/bin/sh -> dash
[joakim@hyperion ~]$ sh -i -l
$ echo "Hello $USER"
Hello joakim
$ echo 'Hello $USER'
Hello $USER

bash on ubuntu

[joakim@hyperion ~]$ echo "Hello $USER"
Hello joakim
[joakim@hyperion ~]$ echo 'Hello $USER'
Hello $USER

zsh on ubuntu

[joakim@hyperion ~]$ zsh -i -l
joakim@hyperion ~ % echo "Hello $USER"
Hello joakim
joakim@hyperion ~ % echo 'Hello $USER'
Hello $USER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using single quotes is ugly/complex because we have to escape literal single quotes outside of the scope of the quotes, so quoting something like my.property='foo bar' (where the single quotes are literal) will end up with the unreadable my.property=\''foo bar'\'

Double quotes a simpler and with 4 characters that need escaping are well enough defined and simple to support.

Copy link
Contributor

@joakime joakime Jul 12, 2023

Choose a reason for hiding this comment

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

The example property my.property='foo bar' would, as if it appeared in start.d/my.ini would mean that the end user wants the single quotes in the value as the end result (meaning if they do properties.get("my.property") they would expect a String with those single-tick characters: "'foo bar'")

The end result would be for strong quotes ...

'-Dmy.property='\''foo bar'\'''

The entire argument is strong quoted, not just a section.

Example:

[joakim@hyperion ~]$ java -Dmy.property='foo bar' -XshowSettings:properties --version 2>&1 | grep my.prop
    my.property = foo bar
[joakim@hyperion ~]$ java '-Dmy.property='\''foo bar'\''' -XshowSettings:properties --version 2>&1 | grep my.prop
    my.property = 'foo bar'

This Strong Quoting also eliminates ALL interpretation of the contents of the quoted string by the shell (on all shells)

From the GNU dash man page (what the standard sh is on Ubuntu)

Single Quotes
Enclosing characters in single quotes preserves the literal meaning
of all the characters (except single quotes, making it impossible
to put single-quotes in a single-quoted string).

Something that Double Quotes does not do, and escaping will need to be SUPER robust (things you are ignoring in this PR will fail on various shells) if we go that route.

Yes, I'm aware that dash only mentions a handful of characters for Double Quotes, the other shells have far more special characters when encountered within Double Quotes (a behavior that Strong Quoting doesn't have)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

@joakime what I meant to say was... thanks for persisting and I understood the issue eventually.

Copy link
Contributor

@joakime joakime Jul 13, 2023

Choose a reason for hiding this comment

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

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

It's not xargs doing that.
It is bash doing that, bash (and zsh) will evaluate on any redirection/pipe before redirection/pipe (unlike dash).
This was a design decision by bash to get around deficiencies in sh not allowing variables to be set on redirection/pipe.

*
* @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)
{
this();
args.add(bin);
if (input == null || input.length() == 0)
gregw marked this conversation as resolved.
Show resolved Hide resolved
return input;

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 == '_'
gregw marked this conversation as resolved.
Show resolved Hide resolved
);
sbordet marked this conversation as resolved.
Show resolved Hide resolved
}

if (!needsQuoting)
return input;

StringBuilder builder = new StringBuilder(input.length() * 2);
builder.append('"');
gregw marked this conversation as resolved.
Show resolved Hide resolved
builder.append(input, 0, --i);

while (i < input.length())
{
char c = input.charAt(i++);
switch (c)
{
case '"':
case '\\':
case'`':
case'$':
gregw marked this conversation as resolved.
Show resolved Hide resolved
builder.append('\\').appendCodePoint(c);
gregw marked this conversation as resolved.
Show resolved Hide resolved
break;
default: builder.appendCodePoint(c);
gregw marked this conversation as resolved.
Show resolved Hide resolved
}
}

builder.append('"');
gregw marked this conversation as resolved.
Show resolved Hide resolved

return builder.toString();
}

/**
Expand All @@ -92,29 +143,25 @@ public void addArg(String arg)
if (arg != null)
{
args.add(arg);
if (commandLine.length() > 0)
commandLine.append(separator);
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
* undefined or empty.
*
* <pre>
* addEqualsArg("-Dname", "value") = "-Dname=value"
* addEqualsArg("-Djetty.home", "/opt/company inc/jetty (7)/") = "-Djetty.home=/opt/company\ inc/jetty\ (7)/"
* addEqualsArg("-Djenkins.workspace", "/opt/workspaces/jetty jdk7/") = "-Djenkins.workspace=/opt/workspaces/jetty\ jdk7/"
* addEqualsArg("-Dstress", null) = "-Dstress"
* addEqualsArg("-Dstress", "") = "-Dstress"
* </pre>
*
* @param name the name
* @param value the value
*/
public void addEqualsArg(String name, String value)
public void addArg(String name, String value)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
if (commandLine.length() > 0)
commandLine.append(separator);
commandLine.append(shellQuoteIfNeeded(name));
if ((value != null) && (value.length() > 0))
{
args.add(name + "=" + value);
commandLine.append('=').append(shellQuoteIfNeeded(value));
}
else
{
Expand All @@ -123,17 +170,31 @@ public void addEqualsArg(String name, String value)
}

/**
* Add a simple argument to the command line.
* <p>
* Will <b>NOT</b> quote/escape arguments that have a space in them.
*
* @param arg the simple argument to add
* @param option the option
* @param name the name
* @param value the value
*/
public void addRawArg(String arg)
public void addArg(String option, String name, String value)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
if (arg != null)
if (commandLine.length() > 0)
commandLine.append(separator);
commandLine.append(option);
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic to add option, name and value to commandLine and args seems different.
Should they not be paired and be equal?
E.g. above we always add option to commandLine, why not to args too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do. All three branches below add option to the args one way or another. The code is a bit convoluted because it is minimal. Let me make it more explicit even if it duplicates a few lines....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that better?

if (name == null || name.length() == 0)
sbordet marked this conversation as resolved.
Show resolved Hide resolved
{
args.add(arg);
args.add(option);
}
else
{
commandLine.append(shellQuoteIfNeeded(name));
if ((value != null) && (value.length() > 0))
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
args.add(option + name + "=" + value);
commandLine.append('=').append(shellQuoteIfNeeded(value));
}
else
{
args.add(option + name);
}
}
}

Expand All @@ -144,20 +205,12 @@ public List<String> getArgs()

@Override
public String toString()
{
return toString(" ");
}

public String toString(String delim)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
StringBuilder buf = new StringBuilder();

for (String arg : args)
{
if (buf.length() > 0)
{
buf.append(delim);
}
buf.append(' ');
buf.append(arg); // we assume escaping has occurred during addArg
}

Expand All @@ -169,23 +222,9 @@ public String toString(String delim)
*
* @return the toString but each arg that has spaces is surrounded by {@code '} (single-quote tick)
*/
public String toQuotedString()
public String toCommandLine()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
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();
return commandLine.toString();
}

public void debug()
Expand Down
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.toCommandLine());
}

if (args.isStopCommand())
Expand Down
Loading