RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

as I did not hear back from you I continued to work on the POSIX file permission support for zipfs and specified/implemented the value of 'null' for zip entries with no permission information associated (vs. UnsupportedOperationException).

I updated the CSR: https://bugs.openjdk.java.net/browse/JDK-8213082
And here is an updated webrev for the change: http://cr.openjdk.java.net/~clanger/webrevs/8213031.5/

I also changed the first part of the module-info documentation for jdk.zipfs, mentioning the URI scheme 'jar' and  corrected the information about how the zip file system provider can be accessed and new zip filesystems can be created.

Please kindly review this.

Thank you and best regards
Christoph

> -----Original Message-----
> From: Langer, Christoph
> Sent: Dienstag, 8. Januar 2019 09:27
> To: 'Alan Bateman' <Alan.Bateman at oracle.com>; Volker Simonis
> <volker.simonis at gmail.com>
> Cc: nio-dev <nio-dev at openjdk.java.net>; OpenJDK Dev list <security-
> dev at openjdk.java.net>; Java Core Libs <core-libs-dev at openjdk.java.net>
> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> Hi Alan, Volker,
> 
> thanks for bringing up these discussion points. I agree that we shall carefully
> revisit that.
> 
> First of all, I'd like to point out that PosixFileAttributeView::readAttributes
> won't ever throw UOE with the proposed changes to zipfs. It should always
> return an object of type PosixFileAttributes which will be an incarnation of
> ZipFileSystem.Entry.
> 
> The returned PosixFileAttributes(ZipFileSystem.Entry) object now
> implements 3 additional methods: owner(), group() and permissions(). I can
> see that UOE should only be thrown if something is not supported by an
> implementation at all. So it perfectly fits to owner() and group() because this
> is simply not implemented in zipfs (yet...). As for permissions, I then agree,
> UOE probably isn't the right thing to do because permissions will now be
> supported by zipfs.
> Still, we need to distinguish between the case where no permission
> information is present vs. an empty set of permissions that was explicitly
> stored. You brought up IOException as an alternative. But I think this is not
> really feasible for the following main reason: IOE is no RuntimeException, so
> it has to be declared for a method when it is thrown.
> PosixFileAttributes::permissions, however, does not declare IOE so far. And I
> don't think we can/want to modify the PosixFileAttributes interface for the
> sake of that zipfs implementation change.
> 
> I think, we should look into using/returning null for "no permission
> information present".
> 
> With that, the point is: What happens if we pass null to another
> PosixFileAttributeView::setPermissions implementation (or to
> Files::setPosixFilePermissions, which ends up there). For zipfs, with the
> proposed changeset, this would work perfectly fine. We translate the null
> value into (int)-1 for the "posixPerms" field of "Entry" and will then not set
> permission information in the zip file. But other places in the JDK that
> implement PosixFileAttributeView need some rework. Those are:
> src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix
> src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFile
> AttributeViewImpl
> 
> And we should amend the specs/doc for the following methods to define
> the handling of the null value as a noop:
> PosixFileAttributeView::setPermissions
> PosixFileAttributes::permissions
> Files::setPosixFilePermissions
> Files::getPosixFilePermissions
> 
> In the implementation I can see that we modify the aforementioned places
> to handle null by translating it to (int)-1 in
> UnixFileModeAttribute::toUnixMode and then using this value as noop in
> 'setMode' resp. 'fchmod'.
> 
> Do you think this is the right way to go? Should we maybe do the spec
> update of the permission methods in a separate change?
> 
> Best regards
> Christoph
> 
> > -----Original Message-----
> > From: Alan Bateman <Alan.Bateman at oracle.com>
> > Sent: Montag, 7. Januar 2019 21:46
> > To: Volker Simonis <volker.simonis at gmail.com>
> > Cc: Langer, Christoph <christoph.langer at sap.com>; nio-dev <nio-
> > dev at openjdk.java.net>; OpenJDK Dev list <security-
> > dev at openjdk.java.net>; Java Core Libs <core-libs-dev at openjdk.java.net>
> > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions
> >
> > On 07/01/2019 19:26, Volker Simonis wrote:
> > > :
> > > We considered this, but it is problematic because it is perfectly
> > > valid to have a file with external file attributes where none of the
> > > Posix attributes is actually set (i.e. an empty set of Posix files
> > > attributes). This wouldn't be distinguishable from the case where a
> > > file has no external file attributes. So it seems we have to resort to
> > > throwing an IOE?
> > >
> > Maybe although it would be a bit awkward to deal with. The issues around
> > this remind me a bit about mounting fat32 file systems on Linux or Unix
> > systems where the fields in the stat structure are populated with
> > default values. PosixFileAttributeView::readAttributes is  essentially
> > the equivalent of a stat call. This might be something to look at for
> > the file owner at least.
> >
> > -Alan