RFR(s): 8240235: jdk.test.lib.util.JarUtils updates jar files incorrectly
volker.simonis at gmail.com
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:
> 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 ).
>  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:
> can I please have a review for this small test fix:
> 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
> 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 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,
> 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
More information about the core-libs-dev