-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Quoting for start arguments #10099
Changes from 2 commits
68209de
a85ccf5
26af0c8
d65ca77
df91bfd
f4a2dea
ff67ade
44c1fe2
845fbb1
61b51ba
ab25cd9
6871906
4ecf521
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
* | ||
* @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(); | ||
} | ||
|
||
/** | ||
|
@@ -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 | ||
{ | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic to add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
} | ||
|
||
|
@@ -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() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ubuntubash
on ubuntuzsh
on ubuntuThere was a problem hiding this comment.
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 unreadablemy.property=\''foo bar'\'
Double quotes a simpler and with 4 characters that need escaping are well enough defined and simple to support.
There was a problem hiding this comment.
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 instart.d/my.ini
would mean that the end user wants the single quotes in the value as the end result (meaning if they doproperties.get("my.property")
they would expect a String with those single-tick characters:"'foo bar'"
)The end result would be for strong quotes ...
The entire argument is strong quoted, not just a section.
Example:
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)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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joakime what I meant to say was... thanks for persisting and I understood the issue eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not
xargs
doing that.It is
bash
doing that,bash
(andzsh
) will evaluate on any redirection/pipe before redirection/pipe (unlikedash
).This was a design decision by
bash
to get around deficiencies insh
not allowing variables to be set on redirection/pipe.