RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)

[Langer, Christoph]

Hi,

here's an update of my webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.1/

I added synchronization to the updating of permCache in ZipUtils.java to avoid concurrent modifications.

As per request from Alan, I'm adding security-dev to get a review from security perspective.

Thanks
Christoph

From: Langer, Christoph
Sent: Freitag, 26. Oktober 2018 17:13
To: core-libs-dev <core-libs-dev at openjdk.java.net>; nio-dev <nio-dev at openjdk.java.net>; 'Xueming Shen' <xueming.shen at oracle.com>
Cc: Schmelter, Ralf <ralf.schmelter at sap.com>; 'Volker Simonis' <volker.simonis at gmail.com>
Subject: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions

Hi,

please review this enhancement of jdk.nio.zipfs to support Posix file permissions.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213031

I had already posted this change as part of a larger fix for "6194856: Zip Files lose ALL ownership and permissions of the files" [1]. With the original proposal I was also addressing the java.util.zip API and the jar tool. However, I've decided to split this endeavor into 2 parts. While I still want to follow up on java.util.zip and jar, I'd like to bring the enhancement to jdk.zipfs in first. Both places don't have dependencies to each other and since zipfs does not change an external API, I guess the bar here is lower. Maybe it is even a candidate for down-porting to jdk11u in the future.

After my fix, zipfs will support the PosixFileAttributeView. Posix file attributes can't be fully supported since owner and group are not handled in zip files. So methods supposed to get these attributes will return an UnsupportedOperationException. But the posix permissions will now be correctly handled, that is stored into and read from the zip file.

@Sherman:
Following suggestions from your review, I removed the test with the binary zip file. I initially thought it is a good idea to test on a well-known file. However, testWriteAndReadArchiveWithPosixPerms tests both, writing a zip file and reading it again so I guess test coverage is quite complete here.
Furthermore, I made some public declarations in ZipUtils package private which should suffice.
I also tried to address your performance concerns with permsToFlags and permsFromFlags. In permsToFlags I will now simply iterate to the set of permissions and add the flags to the return value. It's probably more performant than the streaming approach and doesn't look much worse in the code. In permsFromFlags I added a cache of permission sets which should avoid constant calls to the streaming. However, as the return value needs to be mutable, I have to clone the cached permissions. Maybe it would have the same or even better performance if we iteratively fill a new set of permissions each time permsToFlags gets called. What do you think?

Do we need a CSR for this patch?

Thanks & Best regards
Christoph

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/055971.html

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20181029/2a072743/attachment.html>