8186464: ZipFile cannot read some InfoZip ZIP64 zip files #452

gnu-andrew · 2024-02-20T20:40:13Z

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, 3dc011b, 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)
Change must not contain extraneous whitespace
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 <fitzsim@redhat.com>

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 · 2024-02-20T20:40:47Z

👋 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 · 2024-02-20T20:41:37Z

This backport pull request has now been updated with issue from the original commit.

gnu-andrew · 2024-02-20T20:44:59Z

/contributor add Thomas Fitzsimmons fitzsim@redhat.com

mlbridge · 2024-02-20T20:45:17Z

Webrevs

00: Full (0adee467)

openjdk · 2024-02-20T20:45:54Z

@gnu-andrew
Contributor Thomas Fitzsimmons <fitzsim@redhat.com> successfully added.

openjdk · 2024-03-13T20:04:55Z

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

tstuefe · 2024-03-20T15:23:45Z

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 · 2024-04-05T14:15:14Z

@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 · 2024-04-05T16:28:28Z

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 · 2024-04-05T16:41:55Z

(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 · 2024-04-05T16:54:01Z

@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 · 2024-04-05T16:54:36Z

I will also investigate whether the zip_util.c changes in @gnu-andrew's commit, 462cd91, should be a separate backport.

fitzsim · 2024-04-05T16:58:42Z

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

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.

fitzsim · 2024-04-05T17:00:58Z

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 · 2024-04-05T17:04:06Z

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 · 2024-04-05T19:10:21Z

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 · 2024-04-08T08:38:46Z

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 · 2024-04-08T16:09:30Z

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 · 2024-04-26T00:56:27Z

@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 · 2024-05-24T01:07:00Z

@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 · 2024-06-11T15:37:22Z

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

bridgekeeper · 2024-07-09T17:54:34Z

@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 · 2024-07-09T18:49:37Z

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 · 2024-08-06T22:11:04Z

openjdk/jdk#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 · 2024-09-03T22:50:59Z

@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 · 2024-10-02T01:15:51Z

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

gnu-andrew and others added 3 commits February 20, 2024 20:30

Backport 02b9452ed39eccdfe3210e65b17d4759333c0f15

462cd91

Fix whitespace issues reported by jcheck

a4239bc

zip_util.c: Hoist end64buf to avoid Windows x86 error C2143

0adee46

openjdk bot changed the title ~~Backport 02b9452ed39eccdfe3210e65b17d4759333c0f15~~ 8186464: ZipFile cannot read some InfoZip ZIP64 zip files Feb 20, 2024

openjdk bot added backport Port of a pull request already in a different code base rfr Pull request is ready for review labels Feb 20, 2024

fitzsim mentioned this pull request Mar 4, 2024

8327440: Fix "bad source file" error during beaninfo generation #465

Closed

4 tasks

gnu-andrew mentioned this pull request Mar 9, 2024

8186464: ZipFile cannot read some InfoZip ZIP64 zip files #445

Closed

4 tasks

bridgekeeper bot closed this Oct 2, 2024

fitzsim mentioned this pull request Oct 2, 2024

8334048: -Xbootclasspath can not read some ZIP64 zip files openjdk/jdk#19678

Closed

3 tasks

8186464: ZipFile cannot read some InfoZip ZIP64 zip files #452

8186464: ZipFile cannot read some InfoZip ZIP64 zip files #452

Uh oh!

Conversation

gnu-andrew commented Feb 20, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Contributors

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Feb 20, 2024

Uh oh!

openjdk bot commented Feb 20, 2024

Uh oh!

gnu-andrew commented Feb 20, 2024

Uh oh!

mlbridge bot commented Feb 20, 2024

Webrevs

Uh oh!

openjdk bot commented Feb 20, 2024

Uh oh!

openjdk bot commented Mar 13, 2024

Uh oh!

tstuefe commented Mar 20, 2024

Uh oh!

jerboaa commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

jerboaa commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

jerboaa commented Apr 5, 2024

Uh oh!

fitzsim commented Apr 5, 2024

Uh oh!

jerboaa commented Apr 8, 2024

Uh oh!

fitzsim commented Apr 8, 2024

Uh oh!

gnu-andrew commented Apr 26, 2024

Uh oh!

bridgekeeper bot commented May 24, 2024

Uh oh!

fitzsim commented Jun 11, 2024

Uh oh!

bridgekeeper bot commented Jul 9, 2024

Uh oh!

fitzsim commented Jul 9, 2024

Uh oh!

fitzsim commented Aug 6, 2024

Uh oh!

bridgekeeper bot commented Sep 3, 2024

Uh oh!

bridgekeeper bot commented Oct 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

gnu-andrew commented Feb 20, 2024 •

edited by openjdk bot

Loading