8186464: ZipFile cannot read some InfoZip ZIP64 zip files

[gnu-andrew]

This is a re-do of #445 as we reached a deadlock where I could not make myself commit author, as I was not the PR author, and Thomas could not make me the author as he was not a Committer (see SKARA-2173). The content remains the same.

What follows is Thomas' introduction from the original PR:

This patch was applied to the Red Hat 1.8.0 RPMs in June 2020, so it has been deployed to Red Hat customers for over three years.

I verified that the patch applies cleanly to jdk8u-dev master. I confirmed that with the fix portion of the patch reverted, the ReadZip.java test portion of the patch produces this exception:

java.lang.RuntimeException: zipfile: zip64 end failed
	at ReadZip.main(ReadZip.java:209)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
	at java.lang.Thread.run(Thread.java:750)

With the fix applied the test passes:

Passed: java/util/zip/ZipFile/ReadZip.java

With the patch applied on top of jdk8u-dev master tip, 3dc011b7ff955f6c1334058f300708412b21a3ad, make test on Fedora 38 x86-64 passes, with:

Test results: passed: 3,122

I also retested the test cases in JDK-8186464 and confirmed that without this backport, they fail, and with the backport they succeed.

Thank you, Thomas

---

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
• [ ] JDK-8186464 needs maintainer approval

Issue

• JDK-8186464: ZipFile cannot read some InfoZip ZIP64 zip files (Bug - P3)

Contributors

• Thomas Fitzsimmons <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 452

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back andrew! 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 backport pull request has now been updated with issue from the original commit.

[gnu-andrew]

/contributor add Thomas Fitzsimmons [email protected]

[mlbridge[bot]]

Webrevs

• 00: Full (0adee467)

[openjdk[bot]]

@gnu-andrew Contributor Thomas Fitzsimmons <[email protected]> successfully added.

[openjdk[bot]]

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

[tstuefe]

I verified that the patch applies cleanly to jdk8u-dev master.

I'm confused. The original patch https://hg.openjdk.org/jdk10/master/rev/723486922bfe touches ZipFile.java, not zip_util.c. How is this a clean apply? AFAICS this patch re-implements part of the original patch.

[jerboaa]

@gnu-andrew @fitzsim I concur with @tstuefe: What are the changes in zip_util.c about? I don't see them in jdk head either... Should that be a separate bug that then needs backporting?

[fitzsim]

Sorry for the delay in replying; I did not notice that @tstuefe had commented. Thanks for reviewing @jerboaa and @tstuefe .

I did the zip_util.c change to fix this Windows build failure (from my notes, because I cannot find the run in GitHub Actions anymore):

2024-02-15T23:16:09.7594742Z zip_util.c
2024-02-15T23:16:09.7614547Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(601) : error C2143: syntax error : missing ';' before 'type'
2024-02-15T23:16:09.7630440Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(602) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7634131Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(602) : warning C4022: 'findEND64' : pointer mismatch for actual parameter 2
2024-02-15T23:16:09.7637070Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7698807Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7704889Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7710221Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7719684Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7725741Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7728339Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7744006Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(604) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7746613Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7768297Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7779205Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7818211Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7820739Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7823157Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7841271Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7843802Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(605) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7850043Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7857203Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7877689Z make[2]: *** [lib/CoreLibraries.gmk:285: /cygdrive/d/a/jdk8u-dev/jdk8u-dev/jdk/build/windows-x86/jdk/objs/libzip/zip_util.obj] Error 2
2024-02-15T23:16:09.7880764Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7901316Z make[2]: *** Waiting for unfinished jobs....
2024-02-15T23:16:09.7904955Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7908669Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7919215Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7922355Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7924744Z d:/a/jdk8u-dev/jdk8u-dev/jdk/jdk/src/share/native/java/util/zip/zip_util.c(606) : error C2065: 'end64buf' : undeclared identifier
2024-02-15T23:16:09.7926720Z zutil.c
2024-02-15T23:16:09.8678783Z make[1]: *** [BuildJdk.gmk:70: libs-only] Error 2
2024-02-15T23:16:09.8979013Z make: *** [/cygdrive/d/a/jdk8u-dev/jdk8u-dev/jdk//make/Main.gmk:117: jdk-only] Error 2
2024-02-15T23:16:09.9840583Z ##[error]Process completed with exit code 1.

The older compiler on that build machine needed the end64buf declaration to be at the start of the function.

I do see end64buf is declared in the same place in zip_util.c on jdk master, so it would make more sense to start there and backport that change separately. But now I wonder why no one else has seen this and fixed it already.

I think my next step should be to change this pull request to remove the zip_util.c part and see if the builders pass. If they do, then this review can proceed. If they don't, then I will file a jdk/master bug with the relevant explanation and pointers to GHA logs.

[fitzsim]

(From what I can tell, I will not receive GitHub email updates just for comments on this pull request because I did not file it -- that is why I did not receive a notification for @tstuefe's initial question. I will try to visit this/refresh this more often, and @jerboaa's mention of me did send a notification. See also https://github.com/orgs/community/discussions/44376)

[jerboaa]

@fitzsim Yes, please remove the zip_util.c changes. Then we can proceed with reviewing. Different question: Is there a reason why you didn't submit this PR for review?

[fitzsim]

I will also investigate whether the zip_util.c changes in @gnu-andrew's commit, https://github.com/openjdk/jdk8u-dev/pull/452/commits/462cd916c26f9745abca2cfd78ed8d1107db9f0c, should be a separate backport.

[fitzsim]

@jerboaa Yes, @gnu-andrew explained it in the first comment of this pull request:

This is a re-do of https://github.com/openjdk/jdk8u-dev/pull/445 as we reached a deadlock where I could not make myself commit author, as I was not the PR author, and Thomas could not make me the author as he was not a Committer (see SKARA-2173). The content remains the same.

[fitzsim]

Basically, @gnu-andrew is the author, but I volunteered to do the submission process, and we are not sure how to represent that situation with the tooling.

[jerboaa]

Basically, @gnu-andrew is the author, but I volunteered to do the submission process, and we are not sure how to represent that situation with the tooling.

I'm afraid, there is not much we can do but for @gnu-andrew to drive this (because of SKARA-2173). The only alternative is to erase some of the credit, which is probably not wanted.

[fitzsim]

I did more research: the C parts of the proposed changes are jdk8u-specific, so there are no equivalent parts of zip_util.c in newer branches from which to backport them. They are there because they avoid the need to backport the entire large JDK9-era rewrite of the Java parts of Zip support to jdk8u.

[jerboaa]

They are there because they avoid the need to backport the entire large JDK9-era rewrite of the Java parts of Zip support to jdk8u.

Do you know which bug did the rewrite? It would be good to have that on record.

[fitzsim]

Here is some public discussion: https://bugzilla.redhat.com/show_bug.cgi?id=1433262#c21 and https://bugzilla.redhat.com/show_bug.cgi?id=1433262#c22

[gnu-andrew]

@fitzsim Yes, please remove the zip_util.c changes. Then we can proceed with reviewing. Different question: Is there a reason why you didn't submit this PR for review?

If you remove the zip_util.c changes, you remove the core of the fix. The zip_util.c changes are equivalent to the ZipFile.java in the 9+ version of this patch. 8u does not have JDK-8145260: "To bring j.u.z.ZipFile's native implementation to Java to remove the expensive jni cost and mmap crash risk" and this is not a suitable candidate for backport to a stable release.

The test and zipfs changes are the same. The reason I didn't upstream this at the time of development was I was unsure that the test was adequately testing the changes, particularly the ZipFileSystem ones. In 9+, the ZipFileSystem is part of the JDK, thanks to JDK-8038500: "(zipfs) Upgrade ZIP provider to be a supported provider" and so I presume is tested by the FileSystem fs = FileSystems.newFileSystem(uri, Map.of()) code in a way it isn't when it's demo code.

Thomas kindly agreed to verify the tests and upstream the patch. I think the 'clean patch' confusion arose from the RPM patch applying cleanly, not the 11u version.

[bridgekeeper[bot]]

@gnu-andrew 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]

I am experimenting to figure out which parts of zip_util.c are still used in jdk master.

[bridgekeeper[bot]]

@gnu-andrew 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]

I am working on test cases for https://bugs.openjdk.org/browse/JDK-8334048. I found some good ones in the original commit, "8186464: ZipFile cannot read some InfoZip ZIP64 zip files", but it turns out they have since been reworked, so I have to follow the history of them to figure out the best way to adapt the logic now.

Assuming the test cases I provide satisfy the reviewers, I can backport 8334048 separately from this one, which will become just about the demo Java Zip implementation in 8.

[fitzsim]

https://github.com/openjdk/jdk/pull/19678 is ready for review if someone who looked at this one wants to take a look at the zip_util.c changes as applied to JDK head. I wrote three new test cases for it. The new tests, if/when backported to jdk8u-dev, will need all the Java-only backports we have discussed here. I am going to wait to see what happens with it first, before getting the Java-only parts ready for this one.

[bridgekeeper[bot]]

@gnu-andrew 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]]

@gnu-andrew 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.