JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder #7709

RogerRiggs · 2022-03-04T23:20:21Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder
JDK-8282989: Incorrect handling of quoted arguments in ProcessBuilder on Windows (CSR)

Reviewers

Brent Christian (@bchristi-git - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7709/head:pull/7709
$ git checkout pull/7709

Update a local copy of the PR:
$ git checkout pull/7709
$ git pull https://git.openjdk.java.net/jdk pull/7709/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7709

View PR using the GUI difftool:
$ git pr show -t 7709

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7709.diff

bridgekeeper · 2022-03-04T23:21:33Z

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-03-04T23:23:30Z

@RogerRiggs The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

src/java.base/windows/classes/java/lang/ProcessImpl.java

mlbridge · 2022-03-10T21:03:14Z

Webrevs

…nd backport

omikhaltsova · 2022-03-28T11:44:56Z

@RogerRiggs when do you plan to merge this patch approximately?

RogerRiggs · 2022-03-28T14:22:00Z

@omikhaltsova Its taken more time than expected to get the reviews needed. Perhaps in a week or so.

test/jdk/java/lang/ProcessBuilder/ArgCheck.java

jaikiran · 2022-03-31T01:56:28Z

test/jdk/java/lang/ProcessBuilder/ArgCheck.java

+                str = str.replace("\r", "");
+                actual = Arrays.asList(str.split("\n"));
+            } catch (IOException ioe) {
+                throw new RuntimeException(ioe.getMessage(), ioe);


Do you think we should also explicitly do a process.destroy here, to force kill the launched process?

jaikiran · 2022-03-31T01:58:07Z

test/jdk/java/lang/ProcessBuilder/ArgCheck.java

+    // Depending on the mode the final backslash may act as an escape that may turn an added quote to a literal quote
+    private static final String SPACE_AND_BACKSLASH = "SPACE AND BACKSLASH\\";
+    private static final char DOUBLE_QUOTE = '"';
+    private static final char NEWLINE = '\n';


Looks like this isn't being used.

jaikiran · 2022-03-31T02:00:02Z

Hello Roger, I don't have much knowledge of the Windows semantics in this area, but the change looks OK to me. Just a few minor notes in the test case.

bchristi-git

The change looks good. I just have a couple small suggestions for the test.

bchristi-git · 2022-04-04T23:10:53Z

test/jdk/java/lang/ProcessBuilder/ArgCheck.java

+
+    /**
+     * If zero arguments are supplied, run the test cases.
+     * If there are arguments, echo them to Stdout.


I think it's worth mentioning that this echoing of the arguments is how the "java invocation" mode is checked -- by spawning an additional copy of ArgCheck.

bchristi-git · 2022-04-04T23:26:14Z

test/jdk/java/lang/ProcessBuilder/ArgCheck.java

+                    errors++;
+                }
+            } else {
+                // skip unmatched cases


Can an "ignored" metric be added, along with "success" and "error" ?
I don't like silently skipping parts of a test.
Logging skipped test cases would work, too, but that might make the test output overly verbose.

Added a count of skipped tests and a final message.

RogerRiggs · 2022-04-18T18:56:42Z

This change required a CSR, now approved.

openjdk · 2022-04-18T18:59:42Z

@RogerRiggs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8282008: Incorrect handling of quoted arguments in ProcessBuilder

Reviewed-by: bchristi

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 133 new commits pushed to the master branch:

ffdeb32: 8284928: Add links from SourceVersion to specific JLS versions
d3d71ea: 8284922: Fix some doc-comment issues on methods with package access in JDK API
6e36c45: 8284923: Update description of SourceVersion.RELEASE_18
c63fabe: 8284935: Improve debug in java.security.jgss
ef25e18: 8283870: jdeprscan --help causes an exception when the locale is ja, zh_CN or de
21ea740: 8284699: Include all image types to the J2DBench.ColorConvertOpTests
e5041ae: 8144030: [macosx] test java/awt/Frame/ShapeNotSetSometimes/ShapeNotSetSometimes.java fails (again)
f5beafa: 8159599: [TEST_BUG] java/awt/Modal/ModalInternalFrameTest/ModalInternalFrameTest.java
21de4e5: 8284681: compiler/c2/aarch64/TestFarJump.java fails with "RuntimeException: for CodeHeap < 250MB the far jump is expected to be encoded with a single branch instruction"
9695283: 8240903: Add test to check that jmod hashes are reproducible
... and 123 more: https://git.openjdk.java.net/jdk/compare/526e73498eef5c7608845501ab4ebef0997a5c0d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

RogerRiggs · 2022-04-18T19:03:10Z

/integrate

openjdk · 2022-04-18T19:04:06Z

Going to push as commit 897d6c0.
Since your change was applied there have been 133 commits pushed to the master branch:

ffdeb32: 8284928: Add links from SourceVersion to specific JLS versions
d3d71ea: 8284922: Fix some doc-comment issues on methods with package access in JDK API
6e36c45: 8284923: Update description of SourceVersion.RELEASE_18
c63fabe: 8284935: Improve debug in java.security.jgss
ef25e18: 8283870: jdeprscan --help causes an exception when the locale is ja, zh_CN or de
21ea740: 8284699: Include all image types to the J2DBench.ColorConvertOpTests
e5041ae: 8144030: [macosx] test java/awt/Frame/ShapeNotSetSometimes/ShapeNotSetSometimes.java fails (again)
f5beafa: 8159599: [TEST_BUG] java/awt/Modal/ModalInternalFrameTest/ModalInternalFrameTest.java
21de4e5: 8284681: compiler/c2/aarch64/TestFarJump.java fails with "RuntimeException: for CodeHeap < 250MB the far jump is expected to be encoded with a single branch instruction"
9695283: 8240903: Add test to check that jmod hashes are reproducible
... and 123 more: https://git.openjdk.java.net/jdk/compare/526e73498eef5c7608845501ab4ebef0997a5c0d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-04-18T19:04:23Z

@RogerRiggs Pushed as commit 897d6c0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder

a7aec35

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 4, 2022

RogerRiggs mentioned this pull request Mar 7, 2022

8282008: Incorrect handling of quoted arguments in ProcessBuilder #7504

Closed

3 tasks

mkartashev reviewed Mar 10, 2022

View reviewed changes

src/java.base/windows/classes/java/lang/ProcessImpl.java Show resolved Hide resolved

Cleanup comment and copyright

a194e43

RogerRiggs marked this pull request as ready for review March 10, 2022 20:53

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Mar 10, 2022

Refactored ArgCheck test to be more readable and easier to maintain a…

7db92ba

…nd backport

jaikiran reviewed Mar 31, 2022

View reviewed changes

test/jdk/java/lang/ProcessBuilder/ArgCheck.java Show resolved Hide resolved

jaikiran reviewed Mar 31, 2022

View reviewed changes

bchristi-git suggested changes Apr 4, 2022

View reviewed changes

RogerRiggs added 2 commits April 5, 2022 15:57

Add count of skipped tests and improve comments

2de6284

Merge branch 'master' into 8282008-quoted-escape

ba737bf

bchristi-git approved these changes Apr 5, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 18, 2022

openjdk bot added the ready Pull request is ready to be integrated label Apr 18, 2022

openjdk bot added the integrated Pull request has been integrated label Apr 18, 2022

openjdk bot closed this Apr 18, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 18, 2022

RogerRiggs deleted the 8282008-quoted-escape branch June 10, 2022 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder #7709

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder #7709

RogerRiggs commented Mar 4, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 4, 2022

openjdk bot commented Mar 4, 2022

mlbridge bot commented Mar 10, 2022 •

edited

Loading

omikhaltsova commented Mar 28, 2022

RogerRiggs commented Mar 28, 2022

jaikiran Mar 31, 2022

jaikiran Mar 31, 2022

jaikiran commented Mar 31, 2022

bchristi-git left a comment

bchristi-git Apr 4, 2022

bchristi-git Apr 4, 2022

RogerRiggs Apr 5, 2022

RogerRiggs commented Apr 18, 2022

openjdk bot commented Apr 18, 2022 •

edited

Loading

RogerRiggs commented Apr 18, 2022

openjdk bot commented Apr 18, 2022

openjdk bot commented Apr 18, 2022

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder #7709

JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder #7709

Conversation

RogerRiggs commented Mar 4, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Mar 4, 2022

openjdk bot commented Mar 4, 2022

mlbridge bot commented Mar 10, 2022 • edited Loading

Webrevs

omikhaltsova commented Mar 28, 2022

RogerRiggs commented Mar 28, 2022

jaikiran Mar 31, 2022

Choose a reason for hiding this comment

jaikiran Mar 31, 2022

Choose a reason for hiding this comment

jaikiran commented Mar 31, 2022

bchristi-git left a comment

Choose a reason for hiding this comment

bchristi-git Apr 4, 2022

Choose a reason for hiding this comment

bchristi-git Apr 4, 2022

Choose a reason for hiding this comment

RogerRiggs Apr 5, 2022

Choose a reason for hiding this comment

RogerRiggs commented Apr 18, 2022

openjdk bot commented Apr 18, 2022 • edited Loading

RogerRiggs commented Apr 18, 2022

openjdk bot commented Apr 18, 2022

openjdk bot commented Apr 18, 2022

RogerRiggs commented Mar 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 10, 2022 •

edited

Loading

openjdk bot commented Apr 18, 2022 •

edited

Loading