8288763: Pack200 extraction failure with invalid size

[backwaterred]

While running some internal testing, a college has discovered a failure related to the Pack200 archive format on the zLinux platform. The problematic code was removed in the current repo by JDK-8234596, however that change was large, and required a CSR. I feel that it would be too risky and cumbersome to backport those changes to jdk11 (and ultimately jdk8 as well), so I'd like to propose this change as a new change to jdk11 rather than via the usual backport process.

Testing

These changes have been run against the failing test on zLinux and the Tier 1 tests completed successfully.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8288763: Pack200 extraction failure with invalid size

Reviewers

• Volker Simonis (@simonis - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev pull/1163/head:pull/1163
$ git checkout pull/1163

Update a local copy of the PR:
$ git checkout pull/1163
$ git pull https://git.openjdk.org/jdk11u-dev pull/1163/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1163

View PR using the GUI difftool:
$ git pr show -t 1163

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/1163.diff

[bridgekeeper]

👋 Welcome back tsteele! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (46488743)

[RealLucy]

Changes look good to me.

I would like to see a short comment, explaining why it is necessary to reset the compressed size. Looking at the class declaration, I see all members being preset with some value. Is the same instance (re)used multiple times?

Additional "issue": I'm not a Reviewer for the jdk-updates project. Someone else will need to jump in here, maybe @TheRealMDoerr

[RealCLanger]

I agree, some more comments could be helpful or at least some explanation of the fix in the JBS bug. Maybe it is a similar issue as was fixed for java.util.zip by @simonis with https://bugs.openjdk.org/browse/JDK-8253952 ?

[simonis]

This is all a little bit confusing. First of all I don't think that this issue is s390 related. I think it should be independent of the platform and only depend on the source zip file and the zlib version used by the JDK. Unless we can't proof that this is indeed s390 related, please remove the "[s390]" tag from the summary to avoid further confusion.

From the JBS issue you've opened I see that you're running JCK tests when you encountered this issue. You can find the source zip files of these tests under tests/api/java_util/jar/Pack200/distributed/samples. Can you please report the following:

• is the libzip.so of your JDK statically or dynamically linked against zlib (i.e. ldd <path-to-jdk>/lib/libzip.so | grep libz)
• the version of the system zlib (i.e. /lib/x86_64-linux-gnu/libz.so.1 -> libz.so.1.2.11)

Then, do the following:

• use zipinfo -l <path-to-samples>/sample0X.jar to verify the compressed size of the entries in the sample jar files
• use unzip <path-to-samples>/sample0X.jar to extract their contents and jar cfm new_sample.jar META-INF/MANIFEST.MF * to regenerate them with your JDK under test
• use zipinfo -l new_sample.jar again to verify the compressed sizes of the entries in the newly generated jar file

In the last step, if you find an entry with a compressed size which is different from the compressed size of that same entry in the original file then the root cause of your problem is indeed JDK-8253952 as @RealCLanger suspected. I that case your fix is also correct and I'll be happy to approve it.

I agree with @RealCLanger that this issue is probably another variant of JDK-8253952 (and that JBS issue has several links to other variants with the same root cause). I just want to make sure that we're really seeing the same issue here and don't have another hidden bug on s390.

[backwaterred]

Thanks for your feedback @simonis, @RealCLanger and @RealLucy.

I would like to see a short comment, explaining why it is necessary to reset the compressed size.

I think this is a good suggestion, and it's on my todo list.

This is all a little bit confusing. First of all I don't think that this issue is s390 related.

This is likely my mistake. A member of the Linux on s390 team reported the issue to me, and I must have assumed it was related to that platform. Sorry for the confusion; I have removed the s390 tag in JBS.

Can you please report the following... Then, do the following

Yes. Though I won't have access to this system until next week. I will report back when I have this information.

[backwaterred]

It took me a bit longer than anticipated to get to this. Below is the output you requested @simonis.

• is the libzip.so of your JDK statically or dynamically linked against zlib (i.e. ldd /lib/libzip.so | grep libz)

Looks like it is dynamically linked:

root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted# ldd ../temurin/lib/libzip.so
...
	libz.so.1 => /lib/s390x-linux-gnu/libz.so.1 (0x000003ffaa900000)

• the version of the system zlib (i.e. /lib/x86_64-linux-gnu/libz.so.1 -> libz.so.1.2.11)

You had the version right on.

 ls -al /lib/s390x-linux-gnu/libz.so.1
lrwxrwxrwx 1 root root 14 Mar 26 18:20 /lib/s390x-linux-gnu/libz.so.1 -> libz.so.1.2.11

Following the remaining steps shows a different compressed size.

root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted# zipinfo -l sample04.jar
Archive:  sample04.jar
Zip file size: 11392 bytes, number of entries: 7
-rw----     2.0 fat        0 bX        2 defN 04-Jan-30 13:34 META-INF/
-rw----     2.0 fat       68 bl       68 defN 04-Jan-30 13:34 META-INF/MANIFEST.MF
-rw----     2.0 fat      943 bl      502 defN 04-Jan-30 13:06 com/sun/java/help/impl/CustomKit$CustomDocument.class
-rw----     2.0 fat     3574 bl     1882 defN 04-Jan-30 13:06 com/sun/java/help/impl/CustomKit$ObjectView1.class
-rw----     2.0 fat      191 bl      154 defN 04-Jan-30 13:06 com/sun/java/help/impl/ViewAwareComponent.class
-rw----     2.0 fat     1009 bl      497 defN 04-Jan-30 13:06 com/sun/java/help/impl/JHSecondaryViewer$1.class
-rw----     2.0 fat    14175 bl     7067 defN 04-Jan-30 13:06 com/sun/java/help/impl/JHSecondaryViewer.class
7 files, 19960 bytes uncompressed, 10172 bytes compressed:  49.0%
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted# mkdir scratch
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted# cd scratch/
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# cp ../sample04.jar .
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# unzip sample04.jar
Archive:  sample04.jar
   creating: META-INF/
  inflating: META-INF/MANIFEST.MF
  inflating: com/sun/java/help/impl/CustomKit$CustomDocument.class
  inflating: com/sun/java/help/impl/CustomKit$ObjectView1.class
  inflating: com/sun/java/help/impl/ViewAwareComponent.class
  inflating: com/sun/java/help/impl/JHSecondaryViewer$1.class
  inflating: com/sun/java/help/impl/JHSecondaryViewer.class
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# #../../temurin/bin/jar cfm new_sample.jar META-INF/MANIFEST.MF *
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# rm sample04.jar
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# ../../temurin/bin/jar cfm new_sample.jar META-INF/MANIFEST.MF *
root@15d4a6cf6145:~/workspace/UnpackDistributedFailureExtracted/scratch# zipinfo -l new_sample.jar
Archive:  new_sample.jar
Zip file size: 13114 bytes, number of entries: 12
-rw----     2.0 fat        0 bX        2 defN 22-Jul-07 13:58 META-INF/
-rw----     2.0 fat       68 bl       71 defN 22-Jul-07 13:58 META-INF/MANIFEST.MF
-rw----     1.0 fat        0 b-        0 stor 22-Jul-07 13:57 com/
-rw----     1.0 fat        0 b-        0 stor 22-Jul-07 13:57 com/sun/
-rw----     1.0 fat        0 b-        0 stor 22-Jul-07 13:57 com/sun/java/
-rw----     1.0 fat        0 b-        0 stor 22-Jul-07 13:57 com/sun/java/help/
-rw----     1.0 fat        0 b-        0 stor 22-Jul-07 13:57 com/sun/java/help/impl/
-rw----     2.0 fat      191 bl      161 defN 04-Jan-30 13:06 com/sun/java/help/impl/ViewAwareComponent.class
-rw----     2.0 fat    14175 bl     7841 defN 04-Jan-30 13:06 com/sun/java/help/impl/JHSecondaryViewer.class
-rw----     2.0 fat     3574 bl     2219 defN 04-Jan-30 13:06 com/sun/java/help/impl/CustomKit$ObjectView1.class
-rw----     2.0 fat      943 bl      548 defN 04-Jan-30 13:06 com/sun/java/help/impl/CustomKit$CustomDocument.class
-rw----     2.0 fat     1009 bl      540 defN 04-Jan-30 13:06 com/sun/java/help/impl/JHSecondaryViewer$1.class
12 files, 19960 bytes uncompressed, 11382 bytes compressed:  43.0%

[simonis]

Hi @backwaterred,
Thanks for running the additional tests, really appreciated.
As I said before, I think your changes are fine and I'll approve them.

If you have some extra time to dig into this issue it would still be interesting to find out why the same zlib version 1.2.11 produces different compressed sizes on x86_64 and s390. Do you know if the native zlib on your system contains some extra changes or enhancements compared to the upstream version? And by the way, what Linux distro did you observe this issue?

One way to find this out would be to build you JDK under test with a statically linked zlib version (i.e. --with-zlib=bundled). This will use the original 1.2.11 zlib sources which are bundled with OpenJDK. If this statically linked JDK behaves differently this would indicate that there's some "secrete sauce" in your platforms native zlib. That doesn't mean that there's something wrong about it. It's just the JDK which is not prepared for it :)

[openjdk]

@backwaterred This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8288763: Pack200 extraction failure with invalid size

Reviewed-by: simonis

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 56 new commits pushed to the master branch:

• c9a7259: 8278067: Make HttpURLConnection default keep alive timeout configurable
• ab4a3e6: 8254318: Remove .hgtags
• 8b56027: 8193462: Fix Filer handling of package-info initial elements
• a27448e: 8266675: Optimize IntHashTable for encapsulation and ease of use
• bfc5746: 8203277: preflow visitor used during lambda attribution shouldn't visit class definitions inside the lambda body
• 65ac123: 8254178: Remove .hgignore
• 6d56b2b: 8289799: Build warning in methodData.cpp memset zero-length parameter
• 4f1ac63: 8279622: C2: miscompilation of map pattern as a vector reduction
• 1e7ff48: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
• 7a076e4: 8247964: All log0() in com/sun/org/slf4j/internal/Logger.java should be private
• ... and 46 more: https://git.openjdk.org/jdk11u-dev/compare/684f12e7fd0f939ccc133aec46ed3fb7d24d5cc8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealLucy, @simonis) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[backwaterred]

Thanks for running the additional tests, really appreciated.
As I said before, I think your changes are fine and I'll approve them.

@simonis Thanks for your review! I appreciate having an extra set of eyes on the changes as well.

Do you know if the native zlib on your system contains some extra changes or enhancements compared to the upstream version? And by the way, what Linux distro did you observe this issue?

Yes 😏 this was discovered on an IBM zLinux machine with a hardware accelerated zlib. It's special sauce galore! The OS is Ubuntu 20.04, and has been seen on RHEL 8.6 as well.

[backwaterred]

/integrate

[openjdk]

@backwaterred
Your change (at version 4648874) is now ready to be sponsored by a Committer.

[simonis]

Do you know if the native zlib on your system contains some extra changes or enhancements compared to the upstream version? And by the way, what Linux distro did you observe this issue?

Yes this was discovered on an IBM zLinux machine with a hardware accelerated zlib. It's special sauce galore! The OS is Ubuntu 20.04, and has been seen on RHEL 8.6 as well.

Thanks for the additional information. You should have said that right from the beginning :)

Interesting to see how the hardware acceleration seems to trade speed for compression ratio (the newly compressed files are all slightly bigger) in the same way the other, purely software based zlib enhancements are doing this (see for example https://github.com/simonis/zlib-bench/blob/master/Results.md).

You should also be aware that this difference in behavior can impact other code, both in the JDK core libraries as well as in user code. JDK-8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size has a detailed explanation of why this problem occurs along with a list of some internal classes which have been fixed and some external libraries which are affected.

Ideally, you should downport JDK-8253952 which fixes the root of the problem and not just a single affected caller like your current fix. But unfortunately, that can't be done straight away because it changes the public API specification and has an associated CSR. Maybe it will be acceptable to only downport the code fix part of JDK-8253952?

[RealCLanger]

/integrate

Hi @backwaterred, in JDK-Updates projects you must not issue /integrate only when a PR is ready. You also need to get maintainer approval by putting the jdk11u-fix-request label in the JBS issue, along with some comment why you're requesting the backport, its testing etc., and waiting for the approval label. This is also true for net new items that aren't a downport of some existing fix as in your case. So, please add the JBS ceremony. Thx.

[backwaterred]

Oh, of course! I was thinking of that as a 'backport thing', but I should have known that it was a wider requirement. Thanks for the head's up. I will add the 'JBS ceremony' :-)

[backwaterred]

Thanks for the additional information. You should have said that right from the beginning :)

Haha. Maybe I should have. My understanding of the zLinux system is growing, so this difference is not one I knew right from the beginning.

Ideally, you should downport JDK-8253952 which fixes the root of the problem and not just a single affected caller like your current fix. But unfortunately, that can't be done straight away because it changes the public API specification and has an associated CSR.

I agree with this assessment. If there is a strong preference to go with the other change minus the API changing code I would be open to that as well.

[RealCLanger]

Oh, of course! I was thinking of that as a 'backport thing', but I should have known that it was a wider requirement. Thanks for the head's up. I will add the 'JBS ceremony' :-)

Great. I approved it now and will also sponsor.

/sponsor

[openjdk]

Going to push as commit 5e1ce54.
Since your change was applied there have been 69 commits pushed to the master branch:

• c63209b: 8262085: Hovering Metal HTML Tooltips in different windows cause IllegalArgExc on Linux
• 4b45646: 8285081: Improve XPath operators count accuracy
• 6496396: 8284944: assert(cnt++ < 40) failed: infinite cycle in loop optimization
• e02ab45: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5
• a10ef71: 8286177: C2: "failed: non-reduction loop contains reduction nodes" assert failure
• 39eecbf: 8271078: jdk/incubator/vector/Float128VectorTests.java failed a subtest
• 8486da7: 8255729: com.sun.tools.javac.processing.JavacFiler.FilerOutputStream is inefficient
• a9c27da: 8236490: Compiler bug relating to @nonnull annotation
• 75519b1: 8285380: Fix typos in security
• ccd3de5: 8287223: C1: Inlining attempt through MH::invokeBasic() with null receiver
• ... and 59 more: https://git.openjdk.org/jdk11u-dev/compare/684f12e7fd0f939ccc133aec46ed3fb7d24d5cc8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@RealCLanger @backwaterred Pushed as commit 5e1ce54.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.