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&pr=7504&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7504&range=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
