Re: RFR: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder [v2]
On Tue, 15 Mar 2022 21:11:31 GMT, Roger Riggs wrote: >> Quoting related changes in https://bugs.openjdk.java.net/browse/JDK-8250568 >> modified the way that >> process builder recognized argument strings, causing some arguments to be >> doubly quoted and malformed. >> >> ProcessBuilder encodes command arguments in two ways, a looser legacy >> encoding >> and stricter encoding that prevents quotes from being misinterpreted. >> The system property jdk.lang.Process.allowAmbiguousCommands controls which >> is in effect. >> >> When the property is "true" or not set, arguments are inserted into the >> Windows command line >> with minimal changes. Arguments containing space or tab are quoted to >> prevent them being split. >> Arguments that start and end with double-quote are left alone. >> Some executables interpret a backslash before the final quote as an escape; >> if the argument >> contains first and last quotes, backslashes are ignored. >> >> When the allowAmbigousCommands property is `false`, care is taken to ensure >> that >> the final quote of an argument is the closing quote for the argument and is >> not >> interpreted as a literal quote by a preceding quote (or an odd number of >> quotes). >> >> The PR includes a test matrix of the cases where an argument with spaces and >> a final backslash >> is passed with each combination of `allowAmbiguousCommands = true and false`, >> launched executable, java, .cmd, and .vbs and when the argument is >> surrounded with double-quotes. >> >> The priority for allowAmbiguousCommands = false is that no argument is split >> or joined to another argument. >> In some cases, backslashes are doubled to prevent a double-quote from being >> interpreted incorrectly. >> The trailing backslash in an argument occurs rarely exception when the >> argument is a directory. >> In that case, the addition of trailing backslashes is benign when the string >> is used as a filesystem path. >> >> See also PR#7504, for background and a proposal. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Refactored ArgCheck test to be more readable and easier to maintain and > backport @RogerRiggs when do you plan to merge this patch approximately? - PR: https://git.openjdk.java.net/jdk/pull/7709
Withdrawn: 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. This pull request has been closed without being integrated. - 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]
> 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
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 => no
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&pr=7504&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7504&range=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]
> 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&pr=7504&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7504&range=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
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&pr=7504&range=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