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

[Volker Simonis]

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