8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory #10383

jaikiran · 2022-09-22T06:34:53Z

Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8294193?

As discussed in a related PR #10266 (comment), this is a genuine bug in the java.nio.file.Files.createDirectories implementation. The commit in this PR fixes that by making sure that links are followed when checking if a Path is a directory during the creation of directories in the createDirectories implementation. The private method which is changed in this PR is only used in the Files.createDirectories and no other place, so this change should not impact any other code. Additionally a new test has been added to reproduce the issue and verify the fix.
tier1, tier2 and tier3 tests passed with this change.

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

Issue

JDK-8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10383

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

Using diff file

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

… existence

bridgekeeper · 2022-09-22T06:36:21Z

👋 Welcome back jpai! 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-09-22T06:39:14Z

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

nio

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.

mlbridge · 2022-09-22T06:50:05Z

Webrevs

DamonFool · 2022-09-22T07:11:57Z

It passed on our machine with this patch.
Thanks for fixing it.

AlanBateman · 2022-09-22T07:43:07Z

I think create a new issue for Files.createDirectories issue or else move the existing issue from hotspot/runtime and change the description.

I plan to review this change but not today because it require digging into some security issues.

For the test then I think it will mean moving the existing tests from Misc.java to the new source file. That will fit with the description that you propose for the new test.

…ies.java

jaikiran · 2022-09-22T09:15:55Z

Hello Alan,

I think create a new issue for Files.createDirectories issue or else move the existing issue from hotspot/runtime and change the description.

I've now created a separate JBS and this PR has been updated to link to that JBS.

For the test then I think it will mean moving the existing tests from Misc.java to the new source file. That will fit with the description that you propose for the new test.

I've updated the PR to move the Files.createDirectories testing from Misc.java to this new test class. I have done some trivial cosmetic/non-functional changes to this moved test (things like printing an assert message if/when any assertion fails in that test).

I plan to review this change but not today because it require digging into some security issues.

Please take your time.

AlanBateman · 2022-09-23T14:15:15Z

test/jdk/java/nio/file/Files/CreateDirectories.java

+        // create one directory
+        Path subdir = tmpdir.resolve("a");
+        Files.createDirectories(subdir);
+        assertTrue(Files.exists(subdir), subdir + " was expected to exist, but didn't");


I realise this is moved from Misc.java but if you replace the "exists" with "isDirectory" then it improve the testing.

AlanBateman · 2022-09-23T14:23:03Z

test/jdk/java/nio/file/Files/CreateDirectories.java

+            throw new SkipException("Symbolic links not supported");
+        }
+        System.out.println("Running tests under directory " + startingDir.toAbsolutePath());
+        final Path fooDir = Files.createDirectory(Path.of(startingDir.toString(), "foo"));


You can simplify this to Files.createDirectory(top.resolve("foo")), same for barDir, and symlink.

AlanBateman · 2022-09-23T14:24:30Z

test/jdk/java/nio/file/Files/CreateDirectories.java

+
+    /**
+     * Creates a symlink which points to a directory that exists. Then calls Files.createDirectories
+     * method passing it the path to the symlink and verifies that no exception gets thrown


I think you can simplify the wording with something like "Test Files.createDirectories symbolic file with an existing directory."

AlanBateman · 2022-09-23T14:24:32Z

test/jdk/java/nio/file/Files/CreateDirectories.java

+    @Test
+    public void testSymlinkDir() throws Exception {
+        // create a temp dir as the "root" in which we will run our tests.
+        final Path startingDir = Files.createTempDirectory(Path.of("."), "8293792");


The tests in this area use TestUtil.createTemporaryDirectory() so it's probably best to keep it consistent if you can.

Also can you rename "startingDir" to "top" as that will make the test much easier to read.

jaikiran · 2022-09-24T06:06:48Z

Hello Alan, I've updated the PR to implement your suggestions in the test. The test continues to pass.

AlanBateman

Thanks for the updates and for taking this issue.

openjdk · 2022-09-24T08:47:25Z

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

8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory

Reviewed-by: alanb

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

3675f4c: 8293252: Shenandoah: ThreadMXBean synchronizer tests crash with aggressive heuristics
543851d: 8289607: Change hotspot/jtreg tests to not use Thread.suspend/resume
e2f8251: 8293618: x86: Wrong code generation in class Assembler
6ecd081: 8294270: make test passes awkward -status:-status:error,fail to jtreg
eca9749: 8288325: [windows] Actual and Preferred Size of AWT Non-resizable frame are different
2e20e7e: 8294271: Remove use of ThreadDeath from make utilities
e45f3d5: 8294281: Allow warnings to be disabled on a per-file basis
664e5b1: 8294187: RISC-V: Unify all relocations for the backend into AbstractAssembler::relocate()
acd75e0: 8294053: Unneeded local variable in handle_safefetch()
0b56b82: 8293991: java/lang/Float/Binary16ConversionNaN.java fails on silent NaN conversions
... and 37 more: https://git.openjdk.org/jdk/compare/e9401e67b3f60206e6a98c1c44367b482506a4de...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.

jaikiran · 2022-09-26T05:15:15Z

Thank you for the review, Alan.

jaikiran · 2022-09-26T05:15:22Z

/integrate

openjdk · 2022-09-26T05:16:42Z

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

3675f4c: 8293252: Shenandoah: ThreadMXBean synchronizer tests crash with aggressive heuristics
543851d: 8289607: Change hotspot/jtreg tests to not use Thread.suspend/resume
e2f8251: 8293618: x86: Wrong code generation in class Assembler
6ecd081: 8294270: make test passes awkward -status:-status:error,fail to jtreg
eca9749: 8288325: [windows] Actual and Preferred Size of AWT Non-resizable frame are different
2e20e7e: 8294271: Remove use of ThreadDeath from make utilities
e45f3d5: 8294281: Allow warnings to be disabled on a per-file basis
664e5b1: 8294187: RISC-V: Unify all relocations for the backend into AbstractAssembler::relocate()
acd75e0: 8294053: Unneeded local variable in handle_safefetch()
0b56b82: 8293991: java/lang/Float/Binary16ConversionNaN.java fails on silent NaN conversions
... and 37 more: https://git.openjdk.org/jdk/compare/e9401e67b3f60206e6a98c1c44367b482506a4de...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-26T05:16:55Z

@jaikiran Pushed as commit 169a5d4.

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

8293792: Follow symlinks in Files.createDirectories for checking file…

ed1c09f

… existence

openjdk bot added the nio nio-dev@openjdk.org label Sep 22, 2022

jaikiran added 2 commits September 22, 2022 12:14

fix whitespace issue in test

65aaf9e

newline at end of test file

daf6514

openjdk bot added the rfr Pull request is ready for review label Sep 22, 2022

jaikiran changed the title ~~8293792: runtime/Dictionary/ProtectionDomainCacheTest.java fails with FileAlreadyExistsException: /tmp~~ 8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory Sep 22, 2022

move Files.createDirectories testing from Misc.java to CreateDirector…

e918d33

…ies.java

AlanBateman reviewed Sep 23, 2022

View reviewed changes

review comments for test

350b3f8

AlanBateman approved these changes Sep 24, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 24, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2022

openjdk bot closed this Sep 26, 2022

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

jaikiran deleted the 8293792 branch September 26, 2022 05:17

8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory #10383

8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory #10383

Uh oh!

Conversation

jaikiran commented Sep 22, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 22, 2022

Uh oh!

openjdk bot commented Sep 22, 2022

Uh oh!

mlbridge bot commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

DamonFool commented Sep 22, 2022

Uh oh!

AlanBateman commented Sep 22, 2022

Uh oh!

jaikiran commented Sep 22, 2022

Uh oh!

AlanBateman Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

AlanBateman Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

AlanBateman Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

AlanBateman Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Sep 24, 2022

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 24, 2022

Uh oh!

jaikiran commented Sep 26, 2022

Uh oh!

jaikiran commented Sep 26, 2022

Uh oh!

openjdk bot commented Sep 26, 2022

Uh oh!

openjdk bot commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jaikiran commented Sep 22, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 22, 2022 •

edited

Loading