8334048: -Xbootclasspath can not read some ZIP64 zip files

[fitzsim]

trafficstars

---

Progress

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

Issue

• JDK-8334048: -Xbootclasspath can not read some ZIP64 zip files (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19678/head:pull/19678
$ git checkout pull/19678

Update a local copy of the PR:
$ git checkout pull/19678
$ git pull https://git.openjdk.org/jdk.git pull/19678/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19678

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19678.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back fitzsim! 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.

[openjdk[bot]]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

[openjdk[bot]]

@fitzsim The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 03: Full - Incremental (27dc9d69)
• 02: Full - Incremental (29090779)
• 01: Full - Incremental (4505172d)
• 00: Full (37f981ce)

[AlanBateman]

Are you planning to add tests? I don't think we should be changing this code with good tests.

[fitzsim]

Are you planning to add tests? I don't think we should be changing this code with good tests.

Yes, I will write a test that replicates the failure. So far I verified the bad-signature condition with GDB, but I agree there should be a test case too. It may take me a while, so I will leave this pull request open in the meantime and then update it with the test case once it is written.

[fitzsim]

It turns out ZIP64 is underrepresented in the test suite in general; for example, https://bugs.openjdk.org/browse/JDK-8185896 is still open.

Using the Java class library alone, I have not been able to produce a ZIP file that demonstrates this issue.

I have ideas about how the standard ZIP classes could be modified to produce small ZIP64 files suitable for testing various code paths.

I was able to produce a small ZIP file using the Info-ZIP command line tool that does demonstrate the issue.

To keep the test setup simple, I inlined the ZIP file as byte constants in BootClassPathZip64Creator, the driver for BootClassPathZip64Test.

Next I am going to check the code coverage provided by this test case for the proposed zip_util.c fix.

[fitzsim]

I saw and looked into this failure:

2024-07-03T09:37:42.6540920Z at ZipSourceCache.testKeySourceMapping(ZipSourceCache.java:102) ... Suppressed: java.nio.file.FileSystemException: 1719999454994-bug8317678.zip: The process cannot access the file because it is being used by another process

but I could not see how zip_util.c could cause that.

I re-ran windows-x64 / test (jdk/tier1 part 2) and that issue resolved itself. I am not sure at this point if this could be caused by the zip_util.c changes, or if it is a transient windows-x64 builder issue.

[fitzsim]

I was able to get the same "invalid CEN header (bad signature)" error for -Xbootclasspath with a ZIP file generated using the ZipFileSystem API, using Map<String, Object> env = Map.of("create", "true", "forceZIP64End", "true"); like readZip64EndZipFs does in test/jdk/java/util/zip/ZipFile/ReadZip.java (https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/ReadZip.java#L230). Given it is the same failure mode, I am inclined not to add a Java part to the test case.

I noticed that readZip64EndInfoZIPStreaming in that same test case uses the approach I found independently for creating a small ZIP64 file, and it also inlines the bytes of that small ZIP file. Therefore the inlining approach in BootClassPathZip64Creator does not regress JDK-8321616.

I still would like to exercise at least one branch of the heuristics conditional with another test case, so I will work on that next. At that point, I suspect one could argue that JDK-8185896 is addressed, unless one would prefer to create another set of expensive tests using large-payload ZIP64 files.

[fitzsim]

This patch set is ready for review. I was able to eliminate the Info-ZIP dependency and shrink the test logic significantly. I added two new test cases, one for non-ZIP64 ZIP files, and one for a ZIP64 file with magic values in CEN, both of which worked before and continue to work with the fix proposed in this pull request.

[fitzsim]

I realized the constructors could instead just be static function calls, so I pushed a small change.

[fitzsim]

I saw a serviceability/jvmti/stress/StackTrace/Suspended/GetStackTraceSuspendedStressTest failure but upon my re-running the job the failure did not happen again. It seems unrelated to the patch set in this pull request.

[bridgekeeper[bot]]

@fitzsim This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[fitzsim]

Commenting in response to the inactivity prompt; I am still watching this one, waiting for a reviewer. I am ready to take action on any feedback I receive.

[gnu-andrew]

I can't review, as I ported the native code changes. On that note, can you make sure I'm credited as co-author using /contributor please?

i.e. /contributor add @gnu-andrew

You may want to also comment on JDK-8185896 which is about adding Zip64 tests.

[fitzsim]

/contributor add @gnu-andrew

[openjdk[bot]]

@fitzsim Contributor Andrew John Hughes <[email protected]> successfully added.

[fitzsim]

Hi @gnu-andrew, I have corrected your contributor status, thank you for the reminder.

I do not have access to comment on https://bugs.openjdk.org/browse/JDK-8185896. Can you please add a comment from me that says:

"I have added to https://github.com/openjdk/jdk/pull/19678 two ZIP64 test cases: one for ZIP64 extensions added implicitly due to one of the CEN fields being too large to represent, and one for ZIP64 extensions that have been added explicitly even though no CEN field required them.

It is not complete coverage -- the "implicit" test only tests the field for the total number of entries. But the coverage they provide may be enough to warrant closing JDK-8185896.

The test cases are fast and portable back to OpenJDK 8, and do not require significant disk space, so they can be run unconditionally as part of the default jtreg test set."

[gnu-andrew]

Hi @gnu-andrew, I have corrected your contributor status, thank you for the reminder.

I do not have access to comment on https://bugs.openjdk.org/browse/JDK-8185896. Can you please add a comment from me that says:

"I have added to #19678 two ZIP64 test cases: one for ZIP64 extensions added implicitly due to one of the CEN fields being too large to represent, and one for ZIP64 extensions that have been added explicitly even though no CEN field required them.

It is not complete coverage -- the "implicit" test only tests the field for the total number of entries. But the coverage they provide may be enough to warrant closing JDK-8185896.

The test cases are fast and portable back to OpenJDK 8, and do not require significant disk space, so they can be run unconditionally as part of the default jtreg test set."

Done. Thanks for adding the attribution.

[fitzsim]

Thank you for reviewing @jerboaa. I will work on implementing your suggestions.

[fitzsim]

I pushed the review suggestions. There were two GitHub Actions failures on macos-aarch64, but they look to be occurrences of this existing bug: https://bugs.openjdk.org/browse/JDK-8247940.

[jaikiran]

Hello @fitzsim, I plan to take a look at this change and also consult with others familiar with the bootclasspath area as well as jar/zip area. I am just noting this now to let you know that the PR hasn't been neglected and I think it will take a while for this to be reviewed given the nature and area of this change.

[fitzsim]

Sounds good @jaikiran.

I can provide some background about this pull request; it was the result of discussions on a related jdk8u-dev pull request (https://github.com/openjdk/jdk8u-dev/pull/452).

Red Hat has been shipping the zip_util.c change proposed here, authored by @gnu-andrew, as a patch to our Red Hat Enterprise Linux java-1.8.0-openjdk packages since mid-2020. In that release all ZIP files the JDK uses are processed by zip_util.c unless someone is using the demo Java implementation. We have had no field reports of issues with the patch.

@gnu-andrew and I would like to upstream ZIP64 support to jdk8u, but the zip_util.c patch only exists within Red Hat's package repository, not anywhere upstream from where it could be backported.

When I checked for the most recent branch that still shipped zip_util.c, I discovered that even mainline still ships it, albeit now only for use in -Xbootclasspath handling. So I split the zip_util.c patch out and added some tests to demonstrate the bug itself and that there should be no regressions, and filed this pull request.

If this is accepted into mainline and gets some broader testing with no issue, then I would like to backport it to 21, 17, 11, and 8.

[fitzsim]

Hi @AlanBateman, when you have a chance can you take a look at the tests I added? Do you think they are sufficient?

[gnu-andrew]

Sounds good @jaikiran.

I can provide some background about this pull request; it was the result of discussions on a related jdk8u-dev pull request (openjdk/jdk8u-dev#452).

Red Hat has been shipping the zip_util.c change proposed here, authored by @gnu-andrew, as a patch to our Red Hat Enterprise Linux java-1.8.0-openjdk packages since mid-2020. In that release all ZIP files the JDK uses are processed by zip_util.c unless someone is using the demo Java implementation. We have had no field reports of issues with the patch.

@gnu-andrew and I would like to upstream ZIP64 support to jdk8u, but the zip_util.c patch only exists within Red Hat's package repository, not anywhere upstream from where it could be backported.

When I checked for the most recent branch that still shipped zip_util.c, I discovered that even mainline still ships it, albeit now only for use in -Xbootclasspath handling. So I split the zip_util.c patch out and added some tests to demonstrate the bug itself and that there should be no regressions, and filed this pull request.

If this is accepted into mainline and gets some broader testing with no issue, then I would like to backport it to 21, 17, 11, and 8.

Just to add to this, while the zip_util.c changes are unique to our 8u patch, they are a conversion to C of the equivalent Java changes made in JDK-8186464, with the intention to backport that change to 8u without the risk of JDK-8145260, which introduces the Java-based implementation. So if there is a risk in this patch, it is likely a risk of a mistake in the translation to C rather than the general gist of the changes, which are present in the JDK's Java zip code.

[bridgekeeper[bot]]

@fitzsim This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@fitzsim This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.