Hi Jaikiran,
thanks for proposing this patch. I just had a look and think the fix in
ZipFileSystem.java is the right thing to do.
As for the test: That could be simplified a bit.
For the path to the test file, you could simply do: final Path jarPath =
Paths.get("zipfs-crc-test.jar");
The test is run in the scratch directory of jtreg, so no reason to go to
java.io.tmpdir.
You can also skip deletion of this test file in the finally block, as the jtreg
scratch directories will be wiped by jtreg eventually.
For the existence check of the test file in line 62, you can simply use
Files.exists.
As for creating the zipfs Filesystem (line 68), you can simply use
FileSystems.newFileSystem(Path, Map), no need to mess around with URIs.
Line 96, where construct the input stream and then in 97, the ZipInputStream, I
suggest you either put both into the try statement or you use ZipInputStream
zis = new ZipInputStream(Files.newInputStream(jarPath)) and then rely on
ZipInputStream cascading the close.
And my last suggestion: you could statically import Assert.assertEquals to
shorten lines 105 and 106.
Thanks
Christoph
> -----Original Message-----
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> On Behalf
> Of Jaikiran Pai
> Sent: Mittwoch, 23. Oktober 2019 13:24
> To: core-libs-dev@openjdk.java.net
> Subject: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads
> to a CRC failure in the generated jar file
>
> Can I please get a review and a sponsor for a potential fix for
> JDK-8232879[1]? The patch is available as a webrev at [2].
>
> What's happening in there is that the
> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream is overriding the
> one arg write(byte b) method and calling the super.write(b) and then
> doing a crc.update. The super.write(b)
> (java.util.zip.DeflaterOutputStream in this case) actually internally
> calls the 3 arg write(b, offset, length) which is overriding by this
> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream. In its
> implementation of write(b, offset, length), in addition to (rightly)
> calling super.write(b, offset, length), this method also updates the CRC
> (again). So this ends up updating the CRC multiple times when the single
> arg write is invoked.
>
> The patch now removes this overridden implementation of write(b) in the
> DeflatingEntryOutputStream so that the call is handled by the
> java.util.zip.DeflaterOutputStream. Although there's no @implNote on
> java.util.zip.DeflaterOutputStream#write(byte b) static that it's
> (always) going to call the 3 arg write(b, offset, length) method, the
> implementation as of now does indeed do that. So I guess, its probably
> OK to rely on that knowledge and get rid of this overridden write(b)
> method instead of coming up with a bit more complicated fix.
>
> The patch also includes a jtreg testcase which reproduces this issues
> and verifies the fix.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8232879
>
> [2] https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/
>
> -Jaikiran
>
