Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]

2022-03-10 Thread Roger Riggs
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]

2022-03-10 Thread Olga Mikhaltsova
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]

2022-03-10 Thread Olga Mikhaltsova
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]

2022-03-10 Thread Maxim Kartashev
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]

2022-03-07 Thread Roger Riggs
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]

2022-03-03 Thread Olga Mikhaltsova
> 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