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

Tue Mar 3 10:27:20 UTC 2020

Thanks for the review Martin, Lance.

On Mon, Mar 2, 2020 at 5:30 PM Lance Andersen <lance.andersen at oracle.com> wrote:
>
> 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
>
>
>
> 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
>
>
>