Java runtime are broken/not working properly with zlib 1.2.12

[jiaz83]

Please allow me to open another ticket for this, but I think it's fairly important to fix/relase this as soon as possible. Please see madler/zlib/#426

Just want to clarify that I don't know if the java<->zlib compatibility issue is caused by this(#426) or some other change and I'm sorry that I'm not familiar enough with c coding to be much of help with finding the underlying cause of it.

[madler]

Please elaborate on "broken" and "not working properly". Do you have examples?

[Sardonimous]

FWIW, here's a test that fails with zlib 1.2.12 with Error decompressing from XZDecDemo.java.xz: XZ Index is corrupt but works with 1.2.11

#!/bin/sh set -v xzjar=xz-1.9.jar rm XZDecDemo.* wget https://raw.githubusercontent.com/mstevens83/xz-java/master/src/XZDecDemo.java wget -O ${xzjar} 'https://search.maven.org/remotecontent?filepath=org/tukaani/xz/1.9/xz-1.9.jar' xzjar=xz-1.9.jar javac -cp $xzjar XZDecDemo.java xz XZDecDemo.java java -cp ".:$xzjar" XZDecDemo XZDecDemo.java.xz >XZDecDemo.java

this is on a Intel(R) Core(TM)2 Duo CPU P8600 @ 2.40GHz, Arch linux

[jiaz83]

@Sardonimous Thank you so much for your help and providing a test. The xz decompression fails within the CRC32 verification code path. Same happens with simple opening/iterating of JarFile with verificiation mode enabled.

[jiaz83]

@madler https://github.com/madler/zlib/issues/426#issuecomment-1100764525 and #613 I think there should be yet another release with this important fix included

[jpdigital]

The same thing happens on an Intel Core i7 950, and according to this entry in the Arch Linux bug tracker on a Core i7 870. But it does not happen on newer Intel CPUs, for example, a Core i7-7500U.

[simonis]

OpenJDK uses an intrinsified version of CRC32 if the CLMUL instruction (shown as PCLMULDQ by procid or pclmulqdq in /proc/cpuinfo) is available and SSE > 2 is supported.

You can use the -Xlog:os+cpu option to double check if the JVM has detected these features:

$ java -Xlog:os+cpu -version
[0.011s][info][os,cpu] CPU: total 8 (initial active 8) (4 cores per cpu, 2 threads per core) family 6 model 142 stepping 10 microcode 0xea, cx8, cmov, fxsr, ht, mmx, 3dnowpref, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, lzcnt, tsc, tscinvbit, avx, avx2, aes, erms, clmul, bmi1, bmi2, rtm, adx, fma, vzeroupper, clflush, clflushopt
...

If the required CPU features are detected at runtime, UseCRC32Intrinsics will be set to true. This means that all CRC32 computations will done by specialized assembly code dynamically generated by the JVM. This is true for interpreted execution as well as for C1 and C2 compiled code. Again, you can check if UseCRC32Intrinsics was set with the follwowing command:

$ java -XX:+PrintFlagsFinal -version | grep -i UseCRC32Intrinsic
     bool UseCRC32Intrinsics                       = true                                   {diagnostic} {default}
...

The error described in this issue can only happen if the JDK is not able to use its own CRC32 intrinsic implementation and and falls back to call into zlib for CRC32 computations. I therefore believe that the CPUs which were reported to produce the failure don't support at least one of the CLMUL or SSE > 2 instructions.

[madler]

@simonis Can you point me to the OpenJDK code for the software fallback?

[simonis]

I know what's going on here now. The problem is that conversions from a 32bit integer to a 64bit unsigned long are doing a sign-extension for negative numbers (see "Integral conversions" in the Implicit conversions section of cppreference.com).

Java has no unsigned integers, so the interface to zlib's crc32 function is defined as follows:

public class CRC32 implements Checksum {
    private int crc;
...
    public void update(int b) {
        crc = update(crc, b);
    }
...
    public void update(byte[] b, int off, int len) {
       ...
        crc = updateBytes(crc, b, off, len);
    }
...
    @IntrinsicCandidate
    private static native int update(int crc, int b);

    private static int updateBytes(int crc, byte[] b, int off, int len) {
        ...
        return updateBytes0(crc, b, off, len);
    }

    @IntrinsicCandidate
    private static native int updateBytes0(int crc, byte[] b, int off, int len);

The implementation of the native methods look like this:

#include <zlib.h>
...
JNIEXPORT jint JNICALL
Java_java_util_zip_CRC32_update(JNIEnv *env, jclass cls, jint crc, jint b)
{
    Bytef buf[1];

    buf[0] = (Bytef)b;
    return crc32(crc, buf, 1);
}

JNIEXPORT jint JNICALL
Java_java_util_zip_CRC32_updateBytes0(JNIEnv *env, jclass cls, jint crc,
                                         jarray b, jint off, jint len)
{
    Bytef *buf = (*env)->GetPrimitiveArrayCritical(env, b, 0);
    if (buf) {
        crc = crc32(crc, buf + off, len);
        (*env)->ReleasePrimitiveArrayCritical(env, b, buf, 0);
    }
    return crc;
}

jint is typedefed to int and crc32() is defined as:

ZEXTERN uLong ZEXPORT crc32   OF((uLong crc, const Bytef *buf, uInt len));

with

typedef unsigned long  uLong; /* 32 bits or more */

Now the problem is that on 64bit architctures unsigned long is 64bit. The call of crc32() in Java_java_util_zip_CRC32_updateByte()/Java_java_util_zip_CRC32_updateBytes0() will lead to an integral conversion of the signed, 32bit integer (i.e. jint) argument crc into an unsigned, 64bit (i.e. uLong) parameter which will trigger a sign extension if crc is negative.

Up to zlib version 1.2.11, crc32() calls crc32_z() which in turn calls crc32_little() on little endian platfroms:

local unsigned long crc32_little(crc, buf, len)
    unsigned long crc;
    const unsigned char FAR *buf;
    z_size_t len;
{
    register z_crc_t c;
    ...
    c = (z_crc_t)crc;
    c = ~c;
    ...
    c = ~c;
    return (unsigned long)c;
}

As you can see, the 64bit, unsigned long crc argument is first converted into the 32bit, unsigned z_crc_t local variable c before c is inverted. This conversion gets rid of the the 32 higher order bits of crc in cases where the the sign extension of crc in Java_java_util_zip_CRC32_updateByte()/Java_java_util_zip_CRC32_updateBytes0() has set them to 1.

In zlib 1.2.12 the implementation of crc32_z() has changed:

unsigned long ZEXPORT crc32_z(crc, buf, len)
    unsigned long crc;
    const unsigned char FAR *buf;
    z_size_t len;
{
    ...
    crc ^= 0xffffffff;
    ...
    crc = (crc >> 8) ^ crc_table[(crc ^ *buf++) & 0xff];
    ...
    /* Return the CRC, post-conditioned. */
    return crc ^ 0xffffffff;
}

As you can see, the unsigned, 64bit crc parameter is XORed with 0xffffffff, but this will leave the higher order bits in place which can then influence the result (i.e. crc >> 8...).

The problem described here has been fixed already in the zlib development branch by https://github.com/madler/zlib/commit/ec3df00224d4b396e2ac6586ab5d25f673caa4c2.

For OpenJDK, this issue can also be solved by properly casting the signed jint crc parameter to unsigned int before calling into zlib's crc32() function because there will be no sign extensions for unsigned 32bit to 64bit integral conversions:

Java_java_util_zip_CRC32_update(JNIEnv *env, jclass cls, jint crc, jint b)
{
    Bytef buf[1];

    buf[0] = (Bytef)b;
    return crc32((unsigned int)crc, buf, 1);
}

Notice how this issue is also related to #426. OpenJDK has always bundled a version of zlib for platforms which don't have a zlib by default. Also, until OpenJDK 8 it was common to statically link the bundled zlib version even on Linux. Probably to overcome problems as described here, OpenJDK has always slightly patched the bundled zlib and hardcoded uLong to 32bit even on 64bit systems:

#ifdef _LP64
typedef unsigned int  uLong;  /* 32 bits or more */
#else
typedef unsigned long  uLong; /* 32 bits or more */
#endif

These changes are still in OpenJDK HEAD for the bundled zlib version 1.2.11 and probably the reason for #426. I think a better solution would be to simply cast the jint crc parameters to unsigned int before calling into zlib as described before. I'll open an OpenJDK issue to propose such a fix.

All that said, I still think it makes sense to create a new zlib release which includes https://github.com/madler/zlib/commit/ec3df00224d4b396e2ac6586ab5d25f673caa4c2 because the current version 1.2.12 will break all OpenJDK versions which dynamically link against it.

[deejayem]

I don't know if the same OpenJDK code is used with GraalVM, but I have also seen errors related to this with Graal and zlib 1.2.12 (but not earlier versions): java.util.zip.ZipException: invalid entry CRC (expected 0x87870f13 but got 0x7878f0ec)

[jiaz83]

@simonis thank you very much for deep diving into this issue!

All that said, I still think it makes sense to create a new zlib release which includes ec3df00 because the current version 1.2.12 will break all OpenJDK versions which dynamically link against it.

I second this

[madler]

@simonis

Thank you for your analysis.

I'll open an OpenJDK issue to propose such a fix.

Was this done?