Hi Sherman,
thanks for your input.
Next week I’ll work on updating my webrev to incorporate your points and I’ll
also draft a CSR.
Best regards
Christoph
From: Xueming Shen <xueming.s...@oracle.com>
Sent: Mittwoch, 10. Oktober 2018 09:17
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR (Enhancement): 6194856: Zip Files lose ALL ownership and
permissions of the files
Hi Langer,
Here are some of my comments.
ZipEntry:
Optional<Set<...>> getPosixPermissions()
(1) Is "Optional" really necessary here. it's a little inconsistent
compared to the rest of
methods in the class. a "null" return might be just fine?
(2) Needs a <p> to separate the first line of the spec from the rest
(3) The wording probably needs more consideration. For example, it might be
more
straightforward to use some similar wording from other methods, for
example
... When read from a ZIP file, this is the posix permission stored in the
{@code external
file attributes} field of the zip file entry's central directory record, if
there is
one.
Also, a @ImplNote might be the better place for "it's not available when
read from ZIS"?
void setPosixPermissions( Set<PosixFilePermission> permissions)
I can see the possible use case of using "null" as a special value to reset
the
permission field (and my proposal of simply returning a null from
getPosixPermissions()
when there is no one, brings some symmetry here?)
ZipUtil:
(1) those POSIX_... constants don't need to be public?
(2) I like the "stream" style implementation of permsTo/FromFlags pair, but
have some
concerns regarding their cost. "stream" is relative expensive (when
you take a look at
those supporting class underneath), as these two are supposed to be
invoked for every
entry inside a zip file, they can be hundreds and thousands ... just
wonder, given most
of the entries likely to have the "same" permission set inside a
"normal" zip/jar file, is it
worth to put in some cache mechanism here, especially for the "get"
method, is it designed
to return a read-only set of permission? (in which we can use some
mechanism here).
That said PosixFileAttributes.getPermissions() actually purposely
returns a modifiable set
of permissions. It might be worth some discussion here, as the
ZipEntry.getPosixPermission()
needs to specify it, if it's going to return a immutable set.
Test: These days it is discouraged to check in a binary zip file as a test
target. It is preferred
to create the testing zip file on the fly.
-Sherman
On 9/25/18, 7:57 AM, Langer, Christoph wrote:
Hi all,
I had asked for opinions regarding adding posix permission support to JDK’s zip
handling libraries and tools [1]. Since I didn’t get clear “no, you can’t do
this” answers, I’m now concretely proposing some enhancements in the area of
java.util.zip, jdk.zipfs and jdk.jartool.
I have reopened the long standing bug report (6194856) which had been set to
“Won’t fix” quite recently for this purpose.
Here are the technical details:
The ZIP format specifications by infozip and pkware ([2] and [3]) do not
explicitly specify the handling of posix file permissions. But it seems to be
common sense that if file attribute compatibility is set to “Unix” in the upper
byte of CEN field “version made by”, the file permissions bits are stored in
CEN field “external file attributes”, byte 3 and 4. My changes try to honor
this in the least obtrusive way. 😊
The following changes are proposed:
java.util.zip.ZipEntry:
it will have an additional field “posixPerms”. A value of -1 means “no
permission information”, positive values will contain the flag values.
There will be 2 new public methods to read/set the permission information:
public Optional<Set<PosixFilePermission>> getPosixPermissions()
public void setPosixPermissions(Set<PosixFilePermission>
permissions)
The usage of type “Optional” reflects that posix permissions are not
necessarily present in a zip file
java.util.zip.ZipFile:
it will have the capability to read the CEN fields and set
posixPerms if available
java.util.zip.ZipOutputStream:
it will store entries with associated posix permissions as unix
type in the CEN, together with the bit mask for the permissions
jdk.jartool:
I propose to add and option "--preserve-posix" or short "-o" to honor the posix
bits that may be stored inside zip/jar files. By default the option is not set
and hence posix permissions are ignored. If the flag is set and the file system
that the jar tool is running on supports posix, posix file permissions that
exist in the file system will be stored in newly created/update archives or
restored to the file system if such information is present in the archive.
jdk.zipfs:
I added the capability for posix file permissions in the
implementation. I decided to support PosixFileAttributes by subclassing
ZipFileAttributes from this superclass as well as subclassing
ZipFileAttributeView from PosixFileAttributeView. However, as
PosixFileAttributes also include groups and owners, I would throw
UnsupportedOperationExceptions in case of invoking methods to handle these
attributes. But this approach seems to be most consistent with e.g.
Files.setPosixFilePermissions and Files.getPosixFilePermissions.
java.nio.file.attribute.PosixFilePermissions:
As this class presents a collection of static helpers, I added
definitions for the posix file bit masks together with methods to convert
between Sets of PosixFilePermission to bit masks containing the according
switches and vice versa. These definitions could theoretically also be moved
inside the java.util.zip or jdk.zipfs implementations where they wouldn’t be
exposed as public APIs. However, in that case the code would probably need to
be duplicated.
I’ve also created two jtreg testcases for both, java.util.zip and jdk.nio.zipfs.
The changes also contain a few further code cleanups.
Here are the links:
Bug: https://bugs.openjdk.java.net/browse/JDK-6194856
Webrev:
http://cr.openjdk.java.net/~clanger/webrevs/6194856.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/6194856.0/>
I’ll write a CSR once there’s some substantial feedback to my endeavor.
Thanks in advance for reviewing/commenting.
Best regards
Christoph
[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055375.html
[2] http://www.info-zip.org/doc/appnote-19970311-iz.zip
[3] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
