8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed #3279

bplb · 2021-03-31T00:22:20Z

Please consider this change to make sun.nio.fs.WindowsFileStore.equals() return true if the root strings of the two objects are equal under case insensitive comparison, and the two WindowsFileStores have the same volume information.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to e1d7b21

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3279

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

Using diff file

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

…was constructed

bridgekeeper · 2021-03-31T00:22:52Z

👋 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 · 2021-03-31T00:24:38Z

@bplb 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 · 2021-03-31T00:29:30Z

Webrevs

AlanBateman · 2021-03-31T10:35:42Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

+                volInfo.fileSystemName().equals(otherInfo.fileSystemName()) &&
+                volInfo.volumeSerialNumber() == otherInfo.volumeSerialNumber();
+        }
+        return false;


The volType == DRIVE_REMOTE case is the reason why we didn't fix this one in the past. Can you summarise that the VolumeInfomation is for this case?

I was not able to test that case yet. Using the VolumeInformation was intended to limit spurious equality.

Can you explain abut DRIVE_REMOTE?

I was not able to test that case yet. Using the VolumeInformation was intended to limit spurious equality.

Can you explain abut DRIVE_REMOTE?

I think the volume type will be DRIVE_REMOTE when the root is a UNC (the constants for the volume types are defined in WIndowsConstants). I'm not 100% sure if the server and share names should be compared with or without regard to case so I think we have a bit more exploration to do there. The DRIVE_FIXED case is clear, and it would be good to at least get that fixed as there has been C:\ vs. c:\ confusion in a number of bug reports.

Based on testing in a Win 10 VM, the volume type is in fact DRIVE_REMOTE. The volume serial number however is zero. From what I can test via testing and reading the server and share names don't care about case.

If we are unsure, for UNC, perhaps the server and share name comparisons can remain case sensitive but the file path component comparison be case insensitive?

Maybe this is something that the Microsoft folks can help us with, it might be that it depends on the transport.

Good idea. I will ask them.

mlbridge · 2021-04-02T00:01:10Z

Mailing list message from Bernd Eckenfels on nio-dev:

Hello,

Is it instead possible to store a normalized version of the store base path (for the purpose of equals but also for display)? This would reduce dependence of file name casing semantics which does not in all cases depend only on the host operating system.

Maybe using lpRootPathName if present in GetVolumeInformation?

Gruss
Bernd
--
http://bernd.eckenfels.net
________________________________
Von: nio-dev <nio-dev-retn at openjdk.java.net> im Auftrag von Brian Burkhalter <bpb at openjdk.java.net>
Gesendet: Thursday, April 1, 2021 9:34:20 PM
An: nio-dev at openjdk.java.net <nio-dev at openjdk.java.net>
Betreff: Re: RFR: 8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed

On Thu, 1 Apr 2021 18:03:30 GMT, Alan Bateman <alanb at openjdk.org> wrote:

If we are unsure, for UNC, perhaps the server and share name comparisons can remain case sensitive but the file path component comparison be case insensitive?

Maybe this is something that the Microsoft folks can help us with, it might be that it depends on the transport.

Good idea. I will ask them.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3279
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210401/2d1f0714/attachment.htm>

mlbridge · 2021-04-02T00:01:11Z

Mailing list message from Brian Burkhalter on nio-dev:

Hello,

On Apr 1, 2021, at 1:29 PM, Bernd Eckenfels <ecki at zusammenkunft.net<mailto:ecki at zusammenkunft.net>> wrote:

Is it instead possible to store a normalized version of the store base path (for the purpose of equals but also for display)? This would reduce dependence of file name casing semantics which does not in all cases depend only on the host operating system.

It?s not clear what is intended here. WindowsFileStore does not have a base path itself. Its root path is the path supplied as a parameter of Files.getFileStore().

Maybe using lpRootPathName if present in GetVolumeInformation?

lpRootPathName is the same as WindowsFileStore.root and is an input to GetVolumeInformationW() [1].

Thanks,

Brian

[1] https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getvolumeinformationw
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210401/ae3e8415/attachment.htm>

mlbridge · 2021-04-02T14:39:06Z

Mailing list message from Alan Bateman on nio-dev:

On 01/04/2021 22:46, Brian Burkhalter wrote:

Hello,

On Apr 1, 2021, at 1:29 PM, Bernd Eckenfels <ecki at zusammenkunft.net
<mailto:ecki at zusammenkunft.net>> wrote:

Is it instead possible to store a normalized version of the store
base path (for the purpose of equals but also for display)? This
would reduce dependence of file name casing semantics which does not
in all cases depend only on the host operating system.

It?s not clear what is intended here. WindowsFileStore does not have a
base path itself. Its root path is the path supplied as a parameter?of
Files.getFileStore().

I think it would be useful to looking at the values of the volInfo field
for both the DRIVE_FIXED and DRIVE_REMOTE cases. I think we need to see
the values of fileSystemName and volumeName to see if we could use them
here.

-Alan.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210402/bb6bee5b/attachment.htm>

bplb · 2021-04-02T16:49:31Z

I already have this information.

C:\ - Win10 system drive

Root: C:\ volName: System, fsName: NTFS, volSN: 1620920694, volType: 3

E:\ - CD drive (VirtualBox Guest Additions)

Root: E:\ volName: VBox_GAs_6.1.18, fsName: CDFS, volSN: -1657170319, volType: 5

Z:\ - VirtualBox shared folder

Root: Z:\ volName: VBOX_VirtualBox, fsName: VBoxSharedFolderFS, volSN: 16777221, volType: 4

\\BPB\Public - Local share created for C:\Users\Public

Root: \\BPB\Public\ volName: System, fsName: NTFS, volSN: 1620920694, volType: 4

\\192.168.0.111\Shared\ - "Remote" share of macOS host /Users/Shared

Root: \\192.168.0.111\Shared\ volName: Macintosh HD - Data, fsName: NTFS, volSN: 0, volType: 4

mlbridge · 2021-04-05T07:16:05Z

Mailing list message from Alan Bateman on nio-dev:

On 02/04/2021 17:53, Brian Burkhalter wrote:

:
4. `\\BPB\Public` - Local share created for `C:\Users\Public`

`Root: \\BPB\Public\
volName: System, fsName: NTFS, volSN: 1620920694, volType: 4`

5. `\\192.168.0.111\Shared` - "Remote" share of macOS host `/Users/Shared`

`Root: \\192.168.0.111\Shared\
volName: Macintosh HD - Data, fsName: NTFS, volSN: 0, volType: 4`

It would be useful for the discussion if you could repeat with UNC
server name and share name in other cases, e.g. \\bpb\public\.

One other thing is that if we do change the equals (and I think we need
to for at least DRIVE_FIXED case) then it will require the hashCode
method to be changed too.

-Alan

bplb · 2021-04-05T17:19:35Z

I already tested with other cases but didn't include it:

Root: \\bpb\Public\
volName: System, fsName: NTFS, volSN: 1620920694, volType: 4

Root: \\BPB\pUBLIC\
volName: System, fsName: NTFS, volSN: 1620920694, volType: 4

Root: \\192.168.0.111\Shared\
volName: Macintosh HD - Data, fsName: NTFS, volSN: 0, volType: 4

Root: \\192.168.0.111\shared\
volName: Macintosh HD - Data, fsName: NTFS, volSN: 0, volType: 4

Good point about hashCode().

bplb · 2021-04-06T21:35:14Z

If we want to constrain the case insensitive comparison to fixed drives, then this might work:

    @Override
    public boolean equals(Object ob) {
        if (ob == this)
            return true;
        if (!(ob instanceof WindowsFileStore))
            return false;
        WindowsFileStore other = (WindowsFileStore)ob;
        if (root.equals(other.root))
            return true;
        if (volType == DRIVE_FIXED && other.volumeType() == DRIVE_FIXED)
            return root.equalsIgnoreCase(other.root);
        return false;
    }

    @Override
    public int hashCode() {
        return volType == DRIVE_FIXED ?
            root.toLowerCase().hashCode() : root.hashCode();
    }

AlanBateman · 2021-04-07T11:29:29Z

If we want to constrain the case insensitive comparison to fixed drives, then this might work:

Yes, although it would be good to extend this to the DRIVE_REMOTE and other cases too, but that requires being confident that it is correct (and I'm not sure we have enough information to say this yet).

An initial change for the DRIVE_FIXED case is okay. It will mean using Locale.ROOT or Locale.US. Also the hash code will need to be cached.

bplb · 2021-04-07T21:50:30Z

(Commit message for 9260dfd intended DRIVE_FIXED, not DRIVE_LOCAL: inadvertently conflated with MNT_LOCAL on macOS.)

bplb · 2021-04-07T21:50:44Z

(Commit message for 9260dfd intended DRIVE_FIXED, not DRIVE_LOCAL: inadvertently conflated with MNT_LOCAL on macOS.)

AlanBateman · 2021-04-08T08:43:40Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

@@ -44,6 +52,9 @@
    private final int volType;
    private final String displayName;   // returned by toString

+    private boolean hasHashCode = false; // as hashCode can be any int


This is buggy. It would be simpler to drop hasHashCode and just race when hash is 0.

AlanBateman · 2021-04-08T16:18:19Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

@@ -221,8 +231,7 @@ public boolean supportsFileAttributeView(String name) {
        if (name.equals("owner"))
            return supportsFileAttributeView(FileOwnerAttributeView.class);
        if (name.equals("user"))
-            return supportsFileAttributeView(UserDefinedFileAttributeView.class);
-        return false;
+            return supportsFileAttributeView(UserDefinedFileAttributeView.class); return false;


Can you revert this change, it's not part of the fix and it makes this method harder to maintain.

I did revert it before but I guess it crept back in ... sigh.

AlanBateman · 2021-04-08T16:18:54Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

@@ -232,12 +241,20 @@ public boolean equals(Object ob) {
        if (!(ob instanceof WindowsFileStore))


We can change this to
if (ob instanced WindowsFileStore other) { ... } and it will avoid the cast.

AlanBateman · 2021-04-08T16:19:56Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

    }

    @Override
    public int hashCode() {
-        return root.hashCode();
+        if (hashCode == 0) { // Don't care about race
+            hashCode = volType == DRIVE_FIXED ?


minor nit but would be a bit easier to read if changed to
hashCode = (volType == DRIVE_FIXED) ? ...

AlanBateman · 2021-04-08T16:20:42Z

test/jdk/java/nio/file/FileStore/Basic.java

+             */
+            FileSystem fs = FileSystems.getDefault();
+            FileStore upper = Files.getFileStore(fs.getPath("C:\\"));
+            FileStore lower = Files.getFileStore(fs.getPath("c:\\"));


Path.of("c:\") can be used if you want.

Will simplify to that in final commit.

AlanBateman · 2021-04-08T16:53:30Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

@@ -44,6 +52,8 @@
    private final int volType;
    private final String displayName;   // returned by toString

+    private int hashCode = 0;
+


Good, I think this is safe and equals/hashCode are consistent. I suspect we can extend this to other (maybe all) volume type but it needs a bit more exploration. Minor nit is that you can't need to initialise hashCode to 0.

I think this issue was specific to the DRIVE_FIXED case so further enhancement can be done under another issue once the information becomes knowd.

I think this issue was specific to the DRIVE_FIXED case so further enhancement can be done under another issue once the information becomes knowd.

That's okay with me.

AlanBateman · 2021-04-09T06:18:22Z

test/jdk/java/nio/file/FileStore/Basic.java

+            /**
+             * Test: FileStore.equals() should not be case sensitive
+             */
+            FileSystem fs = FileSystems.getDefault();


I assume this line is no longer needed.

openjdk · 2021-04-09T06:19:52Z

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

8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed

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

1ca4abe: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI
06e6b1f: 8259242: Remove ProtectionDomainSet_lock
9bb1863: 8260923: Add more tests for SSLSocket input/output shutdown
33fa855: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()
b3782ea: 8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related
f7a6c63: 8259822: [PPC64] Support the prefixed instruction format added in POWER10
a45733f: 8264779: Fix doclint warnings in java/nio
3e57924: 8264885: Fix the code style of macro in aarch64_neon_ad.m4
051c117: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
1c6b113: 8264513: Cleanup CardTableBarrierSetC2::post_barrier
... and 304 more: https://git.openjdk.java.net/jdk/compare/6aa28b3bdb78eb041a963d659d61e1622bc43ef8...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.

bplb · 2021-04-09T15:40:53Z

/integrate

openjdk · 2021-04-09T15:42:22Z

@bplb Since your change was applied there have been 315 commits pushed to the master branch:

6de0bb2: 8232861: (fc) FileChannel.force fails on WebDAV file systems (macOS)
1ca4abe: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI
06e6b1f: 8259242: Remove ProtectionDomainSet_lock
9bb1863: 8260923: Add more tests for SSLSocket input/output shutdown
33fa855: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()
b3782ea: 8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related
f7a6c63: 8259822: [PPC64] Support the prefixed instruction format added in POWER10
a45733f: 8264779: Fix doclint warnings in java/nio
3e57924: 8264885: Fix the code style of macro in aarch64_neon_ad.m4
051c117: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
... and 305 more: https://git.openjdk.java.net/jdk/compare/6aa28b3bdb78eb041a963d659d61e1622bc43ef8...master

Your commit was automatically rebased without conflicts.

Pushed as commit cc54de7.

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

turbanoff · 2021-04-09T22:24:03Z

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java

+            hashCode = (volType == DRIVE_FIXED) ?
+                root.toLowerCase(Locale.ROOT).hashCode() : root.hashCode();
+        }
+        return hashCode;


As I know, to make data race benign, field should be read only once. But there is second read which can still return 0

You can are right, missed in the final review (as this one went through many iterations).

mlbridge · 2021-04-12T22:30:58Z

Mailing list message from Brian Burkhalter on nio-dev:

On Apr 9, 2021, at 11:46 PM, Alan Bateman <alanb at openjdk.java.net<mailto:alanb at openjdk.java.net>> wrote:

On Fri, 9 Apr 2021 22:24:03 GMT, Andrey Turbanov <github.com+741251+turbanoff at openjdk.org<mailto:github.com+741251+turbanoff at openjdk.org>> wrote:

Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:

8264400: Remove vestigial FileSystems.getDefault()

src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java line 257:

255: root.toLowerCase(Locale.ROOT).hashCode() : root.hashCode();
256: }
257: return hashCode;

As I know, to make data race benign, field should be read only once. But there is second read which can still return `0`

You can are right, missed in the final review (as this one went through many iter

Filed JDK-8265100 to address this oversight.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210412/af4c4863/attachment.htm>

8264400: (fs) WindowsFileStore equality depends on how the FileStore …

3058789

…was constructed

openjdk bot added the rfr Pull request is ready for review label Mar 31, 2021

openjdk bot added the nio nio-dev@openjdk.org label Mar 31, 2021

AlanBateman reviewed Mar 31, 2021

View reviewed changes

8264400: Constrain to DRIVE_LOCAL case

9260dfd

AlanBateman reviewed Apr 8, 2021

View reviewed changes

8264400: Remove hasHashCode and race on hashCode == 0

c9e34e4

AlanBateman reviewed Apr 8, 2021

View reviewed changes

8264400: Clean up equals() and hashCode()

7849c7e

AlanBateman reviewed Apr 8, 2021

View reviewed changes

8264400: Don't initialize hashCode; use Path.of() in test

e1d7b21

AlanBateman reviewed Apr 9, 2021

View reviewed changes

AlanBateman approved these changes Apr 9, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 9, 2021

8264400: Remove vestigial FileSystems.getDefault()

a53d10f

openjdk bot closed this Apr 9, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 9, 2021

turbanoff reviewed Apr 9, 2021

View reviewed changes

bplb deleted the WindowsFileStore-equals-8264400 branch April 12, 2021 22:32

		@@ -232,12 +241,20 @@ public boolean equals(Object ob) {
		if (!(ob instanceof WindowsFileStore))

8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed #3279

8264400: (fs) WindowsFileStore equality depends on how the FileStore was constructed #3279

Conversation

bplb commented Mar 31, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 31, 2021

openjdk bot commented Mar 31, 2021

mlbridge bot commented Mar 31, 2021 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Apr 2, 2021

mlbridge bot commented Apr 2, 2021

mlbridge bot commented Apr 2, 2021

bplb commented Apr 2, 2021 • edited Loading

mlbridge bot commented Apr 5, 2021

bplb commented Apr 5, 2021

bplb commented Apr 6, 2021

AlanBateman commented Apr 7, 2021

bplb commented Apr 7, 2021

bplb commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 9, 2021 • edited Loading

bplb commented Apr 9, 2021

openjdk bot commented Apr 9, 2021

turbanoff Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Apr 12, 2021

bplb commented Mar 31, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 31, 2021 •

edited

Loading

bplb commented Apr 2, 2021 •

edited

Loading

openjdk bot commented Apr 9, 2021 •

edited

Loading

turbanoff Apr 9, 2021 •

edited

Loading