8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index

[bplb]

For a String “s”, s.indexOf(int) can never return a value >= s.length() so change the check

        int pos = syntaxAndInput.indexOf(':');
        if (pos <= 0 || pos == syntaxAndInput.length())

to

        if (pos <= 0)

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request to be approved

Issues

• JDK-8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
• JDK-8291730: (fs) FileSystem.getPathMatcher does not check for ":" at last index (CSR)

Reviewers

• Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 291ef674
• Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to b9de2604
• Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to b9de2604
• Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to b9de2604

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9595/head:pull/9595
$ git checkout pull/9595

Update a local copy of the PR:
$ git checkout pull/9595
$ git pull https://git.openjdk.org/jdk pull/9595/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9595

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9595.diff

[bridgekeeper]

👋 Welcome back bpb! 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]

@bplb The following labels will be automatically applied to this pull request:

• core-libs
• nio

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

[mlbridge]

Webrevs

• 05: Full - Incremental (b0eab55f)
• 04: Full - Incremental (b9de2604)
• 03: Full - Incremental (1af2a4b4)
• 02: Full - Incremental (ff404d4b)
• 01: Full - Incremental (5765fa1c)
• 00: Full (291ef674)

[naotoj]

Looks good, Brian.

[openjdk]

@bplb 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:

8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index

Reviewed-by: naoto, rriggs, alanb, lancea

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 202 new commits pushed to the master branch:

• 7739843: 8288633: The ICC_ColorSpace.fromCIEXYZ method uses the wrong rendering intent
• eb8b789: 8292061: ProblemList serviceability/attach/ConcAttachTest.java on linux-all
• 08274e6: 8290975: Minor cleanup could be done in javax.security
• 7db5abd: 8282642: vmTestbase/gc/gctests/LoadUnloadGC2/LoadUnloadGC2.java fails intermittently with exit code 1
• 124fc4a: 8290180: Convert com/sun/management/UnixOperatingSystemMXBean/GetOpenFileDescriptorCount.sh to jtreg java version
• 4591937: 8290357: Drop HeapRegion::marked_bytes()
• 7676be8: 8291037: Move PLAB resizing mechanism to G1EvacStats
• 861cc67: 8291897: TerminatingThreadLocal(s) not registered from virtual thread(s)
• 8d88be2: 8291459: JVM crash with GenerateOopMap::error_work(char const*, __va_list_tag*)
• 8a804f6: 8291454: Missing check for JLI C runtime library in CoreLibraries.gmk
• ... and 192 more: https://git.openjdk.org/jdk/compare/3d3e3df8f0845d1ce1776ef37b4a2b39461a328a...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]

Please update the issue title and description and PR title and description to match the new understanding.

The IAE message in JrtFileSystem wording is a bit like it is leftover debugging output.
The messages for all 4 IAE could be something as simple as "missing pattern".

[bplb]

The IAE message in JrtFileSystem wording is a bit like it is leftover debugging output. The messages for all 4 IAE could be something as simple as "missing pattern".

Not sure a message is needed. At the other extreme one could say "missing syntax" for ":pattern" and "missing pattern" for "syntax:". Or it could be "Parameter does not take the form: syntax:pattern".

[bplb]

Is there anything else to be done in this PR? Thanks.

[AlanBateman]

Is there anything else to be done in this PR? Thanks.

It looks like this issue has changed from the original bug report and now rejects the empty pattern with IAE. This is a behavior change (and probably the right behavior) but I think will require a small update to the javadoc to make it clear that both an empty syntax and empty pattern are illegal.

[bplb]

[...] I think will require a small update to the javadoc to make it clear that both an empty syntax and empty pattern are illegal.

I read the existing specification

     * @throws  IllegalArgumentException
     *          If the parameter does not take the form: {@code syntax:pattern}

as covering both cases, but I suppose one could explicitly state that "neither syntax nor pattern may be empty."

[AlanBateman]

I read the existing specification

     * @throws  IllegalArgumentException
     *          If the parameter does not take the form: {@code syntax:pattern}

as covering both cases, but I suppose one could explicitly state that "neither syntax nor pattern may be empty."

Alternatively, change "where ':' stands for itself" to "where syntax and pattern components are non-empty strings and ':' stands for itself" sot that it's up front when the input string is described.

[bplb]

Alternatively, change "where ':' stands for itself" to "where syntax and pattern components are non-empty strings and ':' stands for itself" so that it's up front when the input string is described.

I think that reads better; will change accordingly.

[RogerRiggs]

The existing code downstream of the check handles an empty glob or regex pattern as matching the empty string.
If I read it correctly, it will now throw an exception instead of not matching. It might be safer to not change that behavior.

[bplb]

The existing code downstream of the check handles an empty glob or regex pattern as matching the empty string. If I read it correctly, it will now throw an exception instead of not matching. It might be safer to not change that behavior.

Indeed for this code

        FileSystem fs = ...;
        PathMatcher pm = fs.getPathMatcher("glob:");
        System.out.println(pm.matches(Path.of("")));

without the proposed change true is printed but with the change an IllegalArgumentException is thrown so it would be safer to change

        if (pos <= 0 || pos == syntaxAndInput.length())

to

        if (pos <= 0)

as originally suggested.

[AlanBateman]

The existing code downstream of the check handles an empty glob or regex pattern as matching the empty string.
If I read it correctly, it will now throw an exception instead of not matching. It might be safer to not change that behavior.

I'm 99% sure the original intention was for the empty pattern to be rejected with IAE. It was an implementation but that it checked if the index is length rather than length-1. That was JDK 7 and changing is now without be behavior change. I can't imagine the impact would be significant but I agree it's not worth it and maybe we just drop the check as per the JBS issue and update the spec to document the long standing behavior.

[bplb]

[...] I agree it's not worth it and maybe we just drop the check as per the JBS issue and update the spec to document the long standing behavior.

Agreed.

[bplb]

/csr

[openjdk]

@bplb has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@bplb please create a CSR request for issue JDK-8290047 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[bplb]

/integrate

[openjdk]

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

• 8d0d9ea: 8291238: JDK can't be built without G1
• aff7689: 8292075: Remove deleted test compiler/rangechecks/TestRangeCheckHoistingScaledIV.java from ProblemList
• 3677b55: 6391806: JLabel and AbstractButton's imageUpdate method should be better specified
• 0ade264: 8290814: Hide G1RootRegions behind G1ConcurrentMark
• f5b3618: 8291970: Add TableStatistics get function to ResourceHashtable
• cbc9040: 8290278: JavaDoc of index parameter of method JTabbedPane.insertTab
• 2712bc3: 8289741: Remove unused imports from DTDBuilder.java
• af243ca: 8265433: IGV: add graph tooltips with properties
• 7739843: 8288633: The ICC_ColorSpace.fromCIEXYZ method uses the wrong rendering intent
• eb8b789: 8292061: ProblemList serviceability/attach/ConcAttachTest.java on linux-all
• ... and 200 more: https://git.openjdk.org/jdk/compare/3d3e3df8f0845d1ce1776ef37b4a2b39461a328a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@bplb Pushed as commit 4040927.

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