Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Thu, 3 Mar 2022 15:38:44 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Reverted addition of the test via echo
Thanks for the followup and confirmation. I'll move #7709 to review from draft.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Thu, 3 Mar 2022 15:38:44 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Reverted addition of the test via echo
Closed due to an alternative fix #7709.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Mon, 7 Mar 2022 16:46:28 GMT, Roger Riggs wrote:
>> Olga Mikhaltsova has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reverted addition of the test via echo
>
> As an alternative fix, please take a look at Draft PR:
> https://github.com/openjdk/jdk/pull/7709.
>
> In the default handling of arguments, the check for what is quoted is
> reverted to prior to 8255068. First and last quotes are sufficient to
> identify a "quoted" string. The check for a backslash ("\") is removed.
> This original check is sufficient for
> `jdk.lang.Process.allowAmbiguousCommands = true`.
>
> For the case where the system property
> `jdk.lang.Process.allowAmbiguousCommands = false`
> and the argument has first and last quotes, a backslash ("\") before the
> final quote must not allow the quote to interpreted as a literal quote and
> merge the following argument. The backslashes will doubled to prevent the
> interpretation of the quote as a literal. This is the correct encoding if
> the command uses the ".exe" encoding, when reparsing the arguments the
> doubled backslashes are reduced to the original contents.
> When the command is using the simpler parsing that does not support literal
> quotes, the backslash before the quote is typically is a trailing backslash
> on a file path and in that case the additional backslash is redundant and has
> no effect on the interpretation of the argument as a directory path.
>
> The PR includes a test of the 12 combinations of invoking an "java"/.exe
> program, a .cmd script, and a Visual Basic script (which uses the .exe rules
> but different command line parser); with and without application quotes and
> compares the actual results with the expected arguments.
@RogerRiggs Sorry for the delay! I also checked, the test-case with VBS, that
raised this issue, successfully workes with your patch. It would be great to
have it asap.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Mon, 7 Mar 2022 16:46:28 GMT, Roger Riggs wrote:
>> Olga Mikhaltsova has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reverted addition of the test via echo
>
> As an alternative fix, please take a look at Draft PR:
> https://github.com/openjdk/jdk/pull/7709.
>
> In the default handling of arguments, the check for what is quoted is
> reverted to prior to 8255068. First and last quotes are sufficient to
> identify a "quoted" string. The check for a backslash ("\") is removed.
> This original check is sufficient for
> `jdk.lang.Process.allowAmbiguousCommands = true`.
>
> For the case where the system property
> `jdk.lang.Process.allowAmbiguousCommands = false`
> and the argument has first and last quotes, a backslash ("\") before the
> final quote must not allow the quote to interpreted as a literal quote and
> merge the following argument. The backslashes will doubled to prevent the
> interpretation of the quote as a literal. This is the correct encoding if
> the command uses the ".exe" encoding, when reparsing the arguments the
> doubled backslashes are reduced to the original contents.
> When the command is using the simpler parsing that does not support literal
> quotes, the backslash before the quote is typically is a trailing backslash
> on a file path and in that case the additional backslash is redundant and has
> no effect on the interpretation of the argument as a directory path.
>
> The PR includes a test of the 12 combinations of invoking an "java"/.exe
> program, a .cmd script, and a Visual Basic script (which uses the .exe rules
> but different command line parser); with and without application quotes and
> compares the actual results with the expected arguments.
@RogerRiggs I believe your patch fixes the use case(s) we are interested in.
Would be good to see it merged into `master`.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Thu, 3 Mar 2022 15:38:44 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Reverted addition of the test via echo
As an alternative fix, please take a look at Draft PR:
https://github.com/openjdk/jdk/pull/7709.
In the default handling of arguments, the check for what is quoted is reverted
to prior to 8255068. First and last quotes are sufficient to identify a
"quoted" string. The check for a backslash ("\") is removed.
This original check is sufficient for `jdk.lang.Process.allowAmbiguousCommands
= true`.
For the case where the system property `jdk.lang.Process.allowAmbiguousCommands
= false`
and the argument has first and last quotes, a backslash ("\") before the final
quote must not allow the quote to interpreted as a literal quote and merge the
following argument. The backslashes will doubled to prevent the interpretation
of the quote as a literal. This is the correct encoding if the command uses
the ".exe" encoding, when reparsing the arguments the doubled backslashes are
reduced to the original contents.
When the command is using the simpler parsing that does not support literal
quotes, the backslash before the quote is typically is a trailing backslash on
a file path and in that case the additional backslash is redundant and has no
effect on the interpretation of the argument as a directory path.
The PR includes a test of the 12 combinations of invoking an "java"/.exe
program, a .cmd script, and a Visual Basic script (which uses the .exe rules
but different command line parser); with and without application quotes and
compares the actual results with the expected arguments.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
> This fix made equal processing of strings such as ""C:\\Program
> Files\\Git\\"" before and after JDK-8250568.
>
> For example, it's needed to execute the following command on Windows:
> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
> it's equal to:
> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
> ""C:\\Program Files\\Git\\"", "Test").start();`
>
> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
> unquoted due to the condition added in JDK-8250568.
>
> private static String unQuote(String str) {
> ..
>if (str.endsWith("\\"")) {
> return str;// not properly quoted, treat as unquoted
> }
> ..
> }
>
>
> that leads to the additional surrounding by quotes in
> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
> to the space inside the string argument.
> As a result the native function CreateProcessW
> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
> quoted argument:
>
> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
> (jdk.lang.Process.allowAmbiguousCommands = true)
> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
> Test
> (jdk.lang.Process.allowAmbiguousCommands = false)
>
>
> Obviously, a string ending with `"\\""` must not be started with `"""` to
> treat as unquoted overwise it’s should be treated as properly quoted.
Olga Mikhaltsova has updated the pull request incrementally with one additional
commit since the last revision:
Reverted addition of the test via echo
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7504/files
- new: https://git.openjdk.java.net/jdk/pull/7504/files/0eceecac..8cbdfe77
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7504=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7504=02-03
Stats: 89 lines in 1 file changed: 0 ins; 89 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/7504.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7504/head:pull/7504
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Thanks for the example, though my question was in the case in which the extra
quotes were not included by the app.
For example,
ProcessBuilder("java.exe", "-classpath", "C:\\New folder\", "Test",
"test");
I thought the troublesome case was specific to VB/WScript being invoked with
".exe" quoting but WSCRIPT interpreting the command line string using simple
quoting with no escapes.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Mon, 28 Feb 2022 21:30:27 GMT, Roger Riggs wrote:
>> Olga Mikhaltsova has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add a new line to the end of test file for JDK-8282008
>
> (I'm still working on a more nuanced fix that works with .exe, .cmd, and with
> allowAmbiguousCommands both true and false).
>
> The suggested workaround was to remove the application quotes and let
> ProcessBuilder do the quoting.
> That resulted in an extra backslash "\" at the end of a file path. In my
> investigation, the extra "\" doesn't prevent the
> string from being correctly used as a directory path in either VisualBasic or
> cmd.exe.
> So I'm curious, in the original application that uncovered this problem, what
> is/was reported as the error?
> Was the original application retested with the workaround?
>
> The case of the backslash at the end of an argument occurs mainly in a
> directory path.
> Yes, the argument is different, but does it make a difference that matters in
> the context in which it appears.
@RogerRiggs
Our use case was something like this `java -classpath "C:\Program
Files\MySQL\JDBC\" ...`. More specifically, while this works after JDK-8250568
(the string ends with `"`)
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder"", "Test",
"test");
this doesn't and, as I understand, shouldn't (the string ends with `"`):
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder\\"", "Test",
"test");
and produces errors like these
Error: Could not find or load main class folder"" Test test
Caused by: java.lang.ClassNotFoundException: folder"" Test test
However, the following still doesn't work, but, I believe, should (the string
ends with `\"`):
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder"", "Test",
"test");
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
(I'm still working on a more nuanced fix that works with .exe, .cmd, and with
allowAmbiguousCommands both true and false).
The suggested workaround was to remove the application quotes and let
ProcessBuilder do the quoting.
That resulted in an extra backslash "\" at the end of a file path. In my
investigation, the extra "\" doesn't prevent the
string from being correctly used as a directory path in either VisualBasic or
cmd.exe.
So I'm curious, in the original application that uncovered this problem, what
is/was reported as the error?
Was the original application retested with the workaround?
The case of the backslash at the end of an argument occurs mainly in a
directory path.
Yes, the argument is different, but does it make a difference that matters in
the context in which it appears.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Actually, this change should be made even more generic because the string might
end with any even number of the backslash characters followed by a
free-standing quote, in which case additional quoting should not be required.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
It is apparent there is no one "correct" way to quote, but one of the key
features of the Java ecosystem has been its backwards compatibility. In that
light, this change allows our clients to continue doing what they did the way
they did it without the need for modification of their Java code or their
(maybe even foreign) native code. FWIW.
Separately from the above, I wonder if this change would make the fix less
controversial?
-if (str.endsWith("\\"")) {
+if (str.endsWith("\\"") && !str.endsWith(""")) {
This way we verify that the end quote is really just an escaped quote, while
correctly identifying escaped backslash as having nothing to do with the
following quote.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
Hi,
as far as I know, on Windows every program can obtain the lpCommandLine
argument, used in the call of CreateProcess() from its parent, by
calling GetCommandLine() and parse that string as it sees fit. This is
in stark contrast with how Unix-like systems create and execute
programs, where the system call execve(2) accepts an array of arguments,
not a single command line.
There are no fixed rules on how to parse the command line, as witnessed
by the different strategies implemented in the C/C++ runtime (which
splits the command line according to the rules outlined in [1] to
populate the argv[] array in main()), the cmd.exe shell, the wscript.exe
runtime, etc.
Consequently, there are no fixed rules on how to encode a command line
(specifically, the lpCommandLine argument to CreateProcess()) because it
really is up to the invoked program to parse it, whether explicitly or
implicitly, according to its own, directly or indirectly implemented
rules. Even a C/C++ console program could ignore the result that the
runtime automatically provides in argv[] and parse the command line
directly, as obtained by GetCommandLine().
Without knowing the parsing rules of the target program, it is not
possible to encode a command line correctly for CreateProcess(). I doubt
there's a "common denominator" which would cover most cases encountered
in practice.
The best we can hope is to implement encoders (and decoders) for
specific, widely used runtimes. (BTW, for the C/C++ runtime I prepared
an implementation mentioned in [2].)
Greetings
Raffaello
[1]
https://docs.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments
[2]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086105.html
On 2022-02-24 20:18, Olga Mikhaltsova wrote:
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
This fix made equal processing of strings such as ""C:\\Program Files\\Git\\""
before and after JDK-8250568.
For example, it's needed to execute the following command on Windows:
`C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
it's equal to:
`new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program
Files\\Git\\"", "Test").start();`
While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
unquoted due to the condition added in JDK-8250568.
private static String unQuote(String str) {
..
if (str.endsWith("\\"")) {
return str;// not properly quoted, treat as unquoted
}
..
}
that leads to the additional surrounding by quotes in
ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
to the space inside the string argument.
As a result the native function CreateProcessW
(src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
quoted argument:
pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
(jdk.lang.Process.allowAmbiguousCommands = true)
pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
Test
(jdk.lang.Process.allowAmbiguousCommands = false)
Obviously, a string ending with `"\\""` must not be started with `"""` to treat
as unquoted overwise it’s should be treated as properly quoted.
Olga Mikhaltsova has updated the pull request incrementally with one additional
commit since the last revision:
Add a new line to the end of test file for JDK-8282008
Roger, writing a test via echo was not a good idea obviously for this particular case
because of the fact well shown in the doc "4. Everyone Parses Differently",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULESMSEX. The task is
more complicated than it seems at the first glance. The same command line correctly
parsed in an app written in C/C++ might be incorrectly parsed in a VBS app etc.
The suggestion not to use the path-argument surroundings with `'"'` doesn't fix the issue
in case of VBS. It leads to a resulting path-argument ending with the doubled backslash in a
VBS app according to the rules "10.1 The WSH Command Line Parameter Parsing Rules",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WSH.
Below there are some experiments with an app attached to JDK-8282008:
NO FIX
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program
Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = \C:\Program
arg[1] = Files\Git1\\\
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program
Files\Git1\\"" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program
arg[1] = Files\Git1\
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program
Files\Git1"" Test
2. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", "C:\\Program
Files\\Git\", "Test").start();
2.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\\
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Roger, writing a test via echo was not a good idea obviously for this
particular case because of the fact well shown in the doc "4. Everyone Parses
Differently",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULESMSEX. The
task is more complicated than it seems at the first glance. The same command
line correctly parsed in an app written in C/C++ might be incorrectly parsed in
a VBS app etc.
The suggestion not to use the path-argument surroundings with `'"'` doesn't fix
the issue in case of VBS. It leads to a resulting path-argument ending with the
doubled backslash in a VBS app according to the rules "10.1 The WSH Command
Line Parameter Parsing Rules",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WSH.
Below there are some experiments with an app attached to JDK-8282008:
NO FIX
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
""C:\\Program Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = \C:\Program
arg[1] = Files\Git1\\\
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program
Files\Git1\\"" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program
arg[1] = Files\Git1\
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program
Files\Git1"" Test
2. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
"C:\\Program Files\\Git\", "Test").start();
2.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\\
arg[1] = Test
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program
Files\Git1\" Test
2.2 allowAmbiguousCommands = true
arg[0] = C:\Program Files\Git1\\
arg[1] = Test
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program
Files\Git1\" Test
FIXED (as in pr)
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
""C:\\Program Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\
arg[1] = Test
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program
Files\Git1" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program Files\Git1\
arg[1] = Test
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program
Files\Git1" Test
```
Reading the code of unQuote() in terms of logic: "no beginning or ending quote,
or too short => not quoted", - and it seems that if all these conditions are
just opposite (starting and ending quotes and long enough) then a string should
be treated as quoted but it's not. One exception was added and it's strange
that it's applied even in case of paired quotes. Is it truly necessary for the
security fix to skip (=to treat as unquoted) a string starting and ending with
`'"'` in case of a `"\\""` tail?
This proposed fix returns back possibility (as was previously) to use
surrounding with `'"'` as an argument mark-up that allows to pass correctly a
path with a space inside in case of VBS. Roger, would you be so kind to take a
look at this small fix once again, please, and to pay attention to VBS parsing
arguments problem?!
-
PR:
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Please close this PR; the proposed change to the application should resolve the
issue.
The issue should be closed as "not-an-issue".
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
ProcessBuilder handles the quoting of arguments with spaces.
In your QuotedArguments.java, just remove the quotes from the first argument to
CheckCase:
```CheckCase[] cases = {
new CheckCase("C:\\Program Files\\Git\", "C:\\Program
Files\\Git\", "true"),
new CheckCase("C:\\Program Files\\Git\", "C:\\Program
Files\\Git\", "false")
};
That worked for me using openjdk version "17.0.2".
Problems with knowing what and if to quote go back a long time.
I'm working on a new [JEP](https://bugs.openjdk.java.net/browse/JDK-8263697) to
handle more of the cases without application intervention.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Roger, thanks for your comments! But in this case how it is possible to present
a path ending with '\' and including a space inside?
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
> This fix made equal processing of strings such as ""C:\\Program
> Files\\Git\\"" before and after JDK-8250568.
>
> For example, it's needed to execute the following command on Windows:
> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
> it's equal to:
> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
> ""C:\\Program Files\\Git\\"", "Test").start();`
>
> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
> unquoted due to the condition added in JDK-8250568.
>
> private static String unQuote(String str) {
> ..
>if (str.endsWith("\\"")) {
> return str;// not properly quoted, treat as unquoted
> }
> ..
> }
>
>
> that leads to the additional surrounding by quotes in
> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
> to the space inside the string argument.
> As a result the native function CreateProcessW
> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
> quoted argument:
>
> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
> (jdk.lang.Process.allowAmbiguousCommands = true)
> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
> Test
> (jdk.lang.Process.allowAmbiguousCommands = false)
>
>
> Obviously, a string ending with `"\\""` must not be started with `"""` to
> treat as unquoted overwise it’s should be treated as properly quoted.
Olga Mikhaltsova has updated the pull request incrementally with one additional
commit since the last revision:
Add a new line to the end of test file for JDK-8282008
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7504/files
- new: https://git.openjdk.java.net/jdk/pull/7504/files/6a2c82f9..0eceecac
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7504=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7504=01-02
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/7504.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7504/head:pull/7504
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v2]
On Fri, 18 Feb 2022 16:07:29 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add test for JDK-8282008
@omikhaltsova Please take another look at the comment above. The fix
incorrectly allows a final double-quote to be escaped, resulting in unbalanced
quotes and possibly allowing an argument to be joined with the next.
The recommendation is for the application to NOT add quotes to arguments and
allow ProcessBuilder to do the necessary quoting.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v2]
> This fix made equal processing of strings such as ""C:\\Program
> Files\\Git\\"" before and after JDK-8250568.
>
> For example, it's needed to execute the following command on Windows:
> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
> it's equal to:
> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
> ""C:\\Program Files\\Git\\"", "Test").start();`
>
> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
> unquoted due to the condition added in JDK-8250568.
>
> private static String unQuote(String str) {
> ..
>if (str.endsWith("\\"")) {
> return str;// not properly quoted, treat as unquoted
> }
> ..
> }
>
>
> that leads to the additional surrounding by quotes in
> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
> to the space inside the string argument.
> As a result the native function CreateProcessW
> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
> quoted argument:
>
> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
> (jdk.lang.Process.allowAmbiguousCommands = true)
> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
> Test
> (jdk.lang.Process.allowAmbiguousCommands = false)
>
>
> Obviously, a string ending with `"\\""` must not be started with `"""` to
> treat as unquoted overwise it’s should be treated as properly quoted.
Olga Mikhaltsova has updated the pull request incrementally with one additional
commit since the last revision:
Add test for JDK-8282008
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7504/files
- new: https://git.openjdk.java.net/jdk/pull/7504/files/721d4023..6a2c82f9
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7504=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7504=00-01
Stats: 88 lines in 1 file changed: 88 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/7504.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7504/head:pull/7504
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder
On Wed, 16 Feb 2022 21:19:04 GMT, Olga Mikhaltsova
wrote:
> This fix made equal processing of strings such as ""C:\\Program
> Files\\Git\\"" before and after JDK-8250568.
>
> For example, it's needed to execute the following command on Windows:
> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
> it's equal to:
> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
> ""C:\\Program Files\\Git\\"", "Test").start();`
>
> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
> unquoted due to the condition added in JDK-8250568.
>
> private static String unQuote(String str) {
> ..
>if (str.endsWith("\\"")) {
> return str;// not properly quoted, treat as unquoted
> }
> ..
> }
>
>
> that leads to the additional surrounding by quotes in
> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
> to the space inside the string argument.
> As a result the native function CreateProcessW
> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
> quoted argument:
>
> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
> (jdk.lang.Process.allowAmbiguousCommands = true)
> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
> Test
> (jdk.lang.Process.allowAmbiguousCommands = false)
>
>
> Obviously, a string ending with `"\\""` must not be started with `"""` to
> treat as unquoted overwise it’s should be treated as properly quoted.
Actually, there's a bit more to this than first meets the eye.
"A double quote mark preceded by a backslash (") is interpreted as a literal
double quote mark (")."
According to:
https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args
That was the reason for the change in JDK-8250568.
So the application supplied quotes combined with the trailing file separator
results in unbalanced quotes.
Without the application supplied quotes, the implementation quotes the string
(because of the embedded space) and doubles up the backslash so it does not
escape the final quote.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder
On Wed, 16 Feb 2022 21:19:04 GMT, Olga Mikhaltsova
wrote:
> This fix made equal processing of strings such as ""C:\\Program
> Files\\Git\\"" before and after JDK-8250568.
>
> For example, it's needed to execute the following command on Windows:
> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
> it's equal to:
> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
> ""C:\\Program Files\\Git\\"", "Test").start();`
>
> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
> unquoted due to the condition added in JDK-8250568.
>
> private static String unQuote(String str) {
> ..
>if (str.endsWith("\\"")) {
> return str;// not properly quoted, treat as unquoted
> }
> ..
> }
>
>
> that leads to the additional surrounding by quotes in
> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
> to the space inside the string argument.
> As a result the native function CreateProcessW
> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
> quoted argument:
>
> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
> (jdk.lang.Process.allowAmbiguousCommands = true)
> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
> Test
> (jdk.lang.Process.allowAmbiguousCommands = false)
>
>
> Obviously, a string ending with `"\\""` must not be started with `"""` to
> treat as unquoted overwise it’s should be treated as properly quoted.
The fix looks right.
The PR should include a test. Can you write a standalone test in
jdk/test/java/lang/ProcessBuilder/... to confirm the fix.
Possibly it could be added to Basic.java but that file is pretty large and
doesn't seem like the correct place to add.
It may be sufficient to invoke echo with that 3rd argument and verify the
output.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder
This fix made equal processing of strings such as ""C:\\Program Files\\Git\\""
before and after JDK-8250568.
For example, it's needed to execute the following command on Windows:
`C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
it's equal to:
`new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
""C:\\Program Files\\Git\\"", "Test").start();`
While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
unquoted due to the condition added in JDK-8250568.
private static String unQuote(String str) {
..
if (str.endsWith("\\"")) {
return str;// not properly quoted, treat as unquoted
}
..
}
that leads to the additional surrounding by quotes in
ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
to the space inside the string argument.
As a result the native function CreateProcessW
(src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
quoted argument:
pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
(jdk.lang.Process.allowAmbiguousCommands = true)
pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
Test
(jdk.lang.Process.allowAmbiguousCommands = false)
Obviously, a string ending with "\\"" must not be started with """ to treat as
unquoted overwise it’s should be treated as properly quoted.
-
Commit messages:
- 8282008: Incorrect handling of quoted arguments in ProcessBuilder
Changes: https://git.openjdk.java.net/jdk/pull/7504/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7504=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8282008
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/7504.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7504/head:pull/7504
PR: https://git.openjdk.java.net/jdk/pull/7504
