RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Lance Andersen]

Hi Jaikiran,

Thank you for your patch.


The change to Zip FS looks good.  I re-worked the test so that it also runs against ZipFile (which uses the CEN vs the LOC) and Zip FS in addition to ZipInputStream for extra coverage.

The webrev can be found at: http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html <http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html>

I have run the patch and test via Mach5 and jdk-tier1 through jdk-tier3 are clean.

Have a good weekend.

Best
Lance
> On Oct 23, 2019, at 10:47 PM, Jaikiran Pai <jai.forums2013 at gmail.com> wrote:
> 
> Thank you Lance.
> 
> -Jaikiran
> On 24/10/19 1:17 AM, Lance Andersen wrote:
>> Hi Jaikiran,
>> 
>> I will take a look and once we are good with the review, I can sponsor it.
>> 
>> Best
>> Lance
>>> On Oct 23, 2019, at 7:24 AM, Jaikiran Pai <jai.forums2013 at gmail.com <mailto:jai.forums2013 at gmail.com>> wrote:
>>> 
>>> 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 <https://bugs.openjdk.java.net/browse/JDK-8232879>
>>> 
>>> [2] https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/ <https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/>
>>> 
>>> -Jaikiran
>>> 
>>> 
>> 
>> <oracle_sig_logo.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>  <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>  <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive 
>> Burlington, MA 01803
>> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>> 
>> 
>> 

 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>