RFR(s): 8240235: jdk.test.lib.util.JarUtils updates jar files incorrectly

[Lance Andersen]

I think the revised patch looks fine

> On Mar 2, 2020, at 10:15 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> On Fri, Feb 28, 2020 at 8:02 PM Martin Buchholz <martinrb at google.com> wrote:
>> 
>> Welcome to the troublesome world of zip implementations!
>> 
> 
> Well, not many remember that the zip format was designed to work
> efficiently with floppy discs :)
> 
>> It looks like by creating a new JarEntry you are discarding possible information from the old one, e.g. you might be discarding whether the input entry was compressed.  If "jar u" does that, I would consider it a bug!
>> 
> 
> You're right. My fix was a little to simple, and can be improved:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2020/8240235.01
> 
> Preserving the time, comment, extra information, compression method
> and size/CRC if the compression method was "stored" is the best we can
> do. And this is actually exactly what "jar -" does (see [1]).
> 
> [1] http://hg.openjdk.java.net/jdk/jdk/file/e04746cec89c/src/jdk.jartool/share/classes/sun/tools/jar/Main.java#l934
> 
>> Arguably there is a missing API that would allow you to copy zip entries from an input to an output completely intact, without having to decompress and recompress.  (might be hard to design!)
>> 
> 
> Yes, I did think about this as well. It might not be that hard at all.
> We could simply add readRawEntry()/wrtiteRawEntry() methods to
> ZipInputStream/ZipOutputStream. We'd have to tweak
> ZipOutputStream.closeEntry() such that it can handle such "raw" writes
> (e.g. it wouldn't be able to check the CRC any more) and various
> methods like "skip()" in ZipInputStream. I think this might be
> interesting not only because it would allow updating zip-files without
> changing untouched entries, but also because it will considerably
> improve the speed of updating (i.e. we would save the time for
> repeatedly inflating and deflating). I'm not sure though how frequent
> the use-case of updating a zip-file really is?
> 
> But that would definitely be a larger enhancement with SE scope. Do
> you think it's worth it?
> 
>> Probably there should be no such utility method as updateJarFile at all.  Perhaps instead the jar/zip code could be refactored so that tests can reuse the same code used by "jar u".
>> 
> 
> I'll leave that as an extra improvement for others :)
> 
>> On Fri, Feb 28, 2020 at 10:50 AM Volker Simonis <volker.simonis at gmail.com> wrote:
>>> 
>>> Hi,
>>> 
>>> can I please have a review for this small test fix:
>>> 
>>> http://cr.openjdk.java.net/~simonis/webrevs/2020/8240235/
>>> https://bugs.openjdk.java.net/browse/JDK-8240235
>>> 
>>> JarUtils.updateJar() and JarUtils.updateJarFile() update jar files by
>>> reading JarEntry-s from a source jar file and writing these exact
>>> JarEntry-s to the destination jar file followed be the inflated and
>>> re-deflated data belonging to this entry.
>>> 
>>> This approach is not correct, because JarEntry contains both, the
>>> compressed and uncompressed size of the corresponding entry. But the
>>> original Defalter which initially compressed that entry can be either
>>> different from the current one or it might have used a different
>>> compression level compared to the current Deflater which re-deflates
>>> the entry data.
>>> 
>>> When the newly created entry is closed, the new compressed size will
>>> be checked against the original compressed size if that was recorded
>>> in the JarEntry and an exception will be thrown, when they differ:
>>> 
>>> java.util.zip.ZipException: invalid entry compressed size (expected
>>> 237 but got 238 bytes)
>>>        at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267)
>>>        at java.base/java.util.zip.ZipOutputStream.putNextEntry(ZipOutputStream.java:192)
>>>        at java.base/java.util.jar.JarOutputStream.putNextEntry(JarOutputStream.java:108)
>>>        at jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:294)
>>>        at jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:252)
>>>        at HasUnsignedEntryTest.start(HasUnsignedEntryTest.java:77)
>>>        at HasUnsignedEntryTest.main(HasUnsignedEntryTest.java:44)
>>>        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>>> Method)
>>>        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>>        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>        at java.base/java.lang.reflect.Method.invoke(Method.java:564)
>>>        at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
>>>        at java.base/java.lang.Thread.run(Thread.java:832)
>>>        Suppressed: java.util.zip.ZipException: invalid entry
>>> compressed size (expected 237 but got 238 bytes)
>>>                at
>>> java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267)
>>>                at
>>> java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360)
>>>                at
>>> java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237)
>>>                at
>>> java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377)
>>>                at jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:280)
>>> 
>>> The fix is trivial. Instead of copying the JarEntry-s verbatim to the
>>> output stream, simply write newly created JarEntry-s to the output
>>> stream which only contain the entry name. This is also the way how
>>> this is handled by the jar tool, when it updates jar files.
>>> 
>>> Thank you and best regards,
>>> Volker

 <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>