RFR: 8314891: Additional Zip64 extra header validation
Please review this PR which improves the Zip64 extra header validation: - Throw a ZipException If the extra len field is 0 and : -- size, csize, or loc offset are not set to 0x -- disk starting number is not set to 0x - We have a valid size for the Zip64 extra header but we are missing the csize or loc fields if they are expected to be part of the header Mach5 tiers 1-3 are clean - Commit messages: - Clean up some minor formatting issues - Additional Zip64 extra header validation Changes: https://git.openjdk.org/jdk/pull/15650/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314891 Stats: 544 lines in 3 files changed: 513 ins; 7 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation
On Sat, 9 Sep 2023 14:33:53 GMT, Lance Andersen wrote: > Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 52: > 50: * starting number is set to 0x or when we have a valid Zip64 Extra > header > 51: * size but missing the corresponding field. > 52: * @run junit MissingZIP64EntriesTest Is this comment accurate? I think we should check 3 cases when the header extra len == 0, len == 8 and len ==16, but still do not contain all required information. - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1322185302
Re: RFR: 8314891: Additional Zip64 extra header validation
On Sat, 9 Sep 2023 14:33:53 GMT, Lance Andersen wrote: > Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 198: > 196: * actual value is stored in the Zip64 Extra Header > 197: */ > 198: private static final int ZIP64_MAGICCOUNT = 0x; Suggestion: private static final int ZIP64_MAGICCOUNT = 0x; - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1322670868
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Added additional tests, along with additional cleanup and refactoring - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/bb962fcf..40504b2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=00-01 Stats: 737 lines in 3 files changed: 578 ins; 99 del; 60 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
On Tue, 12 Sep 2023 08:47:03 GMT, Andrey Turbanov wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added additional tests, along with additional cleanup and refactoring > > test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 198: > >> 196: * actual value is stored in the Zip64 Extra Header >> 197: */ >> 198: private static final int ZIP64_MAGICCOUNT = 0x; > > Suggestion: > > private static final int ZIP64_MAGICCOUNT = 0x; Removed the extra space. thank you for pointing it out - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1326169219
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
On Mon, 11 Sep 2023 23:42:37 GMT, Sergey Bylokhov wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added additional tests, along with additional cleanup and refactoring > > test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 52: > >> 50: * starting number is set to 0x or when we have a valid Zip64 Extra >> header >> 51: * size but missing the corresponding field. >> 52: * @run junit MissingZIP64EntriesTest > > Is this comment accurate? I think we should check 3 cases when the header > extra len == 0, len == 8 and len ==16, but still do not contain all required > information. Clarified the comment to make it a bit clearer and also added additional tests - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1326168515
Re: RFR: 8314891: Additional Zip64 extra header validation [v3]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Remove tab(s) from comment - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/40504b2c..24b159d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v4]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into JDK-8314891 - Remove tab(s) from comment - Added additional tests, along with additional cleanup and refactoring - Clean up some minor formatting issues - Additional Zip64 extra header validation - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/24b159d8..afc9d7d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=02-03 Stats: 89465 lines in 2912 files changed: 37847 ins; 14022 del; 37596 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v5]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' into JDK-8314891 - Merge branch 'master' into JDK-8314891 - Remove tab(s) from comment - Added additional tests, along with additional cleanup and refactoring - Clean up some minor formatting issues - Additional Zip64 extra header validation - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/afc9d7d0..f64a70c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=03-04 Stats: 56 lines in 3 files changed: 18 ins; 1 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v6]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Revamp isZip64ExtBlockSizeValid - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/f64a70c3..bbc5325d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=04-05 Stats: 61 lines in 3 files changed: 14 ins; 13 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v7]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add missing space - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/bbc5325d..208a5ecc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
On Wed, 8 Nov 2023 17:27:19 GMT, Lance Andersen wrote: >> @LanceAndersen >> >> I noticed that this PR did not update `ZipInputStream.readLOC` to perform >> consistency validation between expected and actual extra field size and >> values. Any particular reason why processing of LOC headers was not made >> consistent with CEN? > >> @LanceAndersen >> >> I noticed that this PR did not update `ZipInputStream.readLOC` to perform >> consistency validation between expected and actual extra field size and >> values. Any particular reason why processing of LOC headers was not made >> consistent with CEN? > > Intentional, as this was a follow on to the updates which were done > previously to the CEN work in August, this is follow on cleanup. > > Updates to ZipInputStream would be done separately under a separate PR or > could be done via your work on 8303866 Hey @LanceAndersen, It was a common practice in obfuscation, to create zips with invalid headers. This change leads to a behavioral change that affects existing work processes. Would it be possible to add an system property to restore the old behavior? - PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-2107932136
Re: RFR: 8314891: Additional Zip64 extra header validation [v7]
On Mon, 18 Sep 2023 13:17:25 GMT, Lance Andersen wrote: >> Please review this PR which improves the Zip64 extra header validation: >> >> - Throw a ZipException If the extra len field is 0 and : >> -- size, csize, or loc offset are set to 0x >> -- disk starting number is set to 0x >> >> - We have a valid size for the Zip64 extra header but we are missing the >> csize or loc fields if they are expected to be part of the header >> >> Mach5 tiers 1-3 are clean > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing space Looks good to me Lance. I note that the extra checks aren't reversed via any sort of system property but given that this fix isn't planned for JDK update releases, that seems fine to me. early testing by frameworks which create/modify custom zip files will be important. - Marked as reviewed by coffeys (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15650#pullrequestreview-1685425990
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
> Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean Lance Andersen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Merge branch 'master' into JDK-8314891 - Add missing space - Revamp isZip64ExtBlockSizeValid - Merge branch 'master' into JDK-8314891 - Merge branch 'master' into JDK-8314891 - Remove tab(s) from comment - Added additional tests, along with additional cleanup and refactoring - Clean up some minor formatting issues - Additional Zip64 extra header validation - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/208a5ecc..255aec85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15650&range=06-07 Stats: 75854 lines in 1953 files changed: 44906 ins; 19381 del; 11567 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
On Mon, 23 Oct 2023 19:12:57 GMT, Lance Andersen wrote: >> Please review this PR which improves the Zip64 extra header validation: >> >> - Throw a ZipException If the extra len field is 0 and : >> -- size, csize, or loc offset are set to 0x >> -- disk starting number is set to 0x >> >> - We have a valid size for the Zip64 extra header but we are missing the >> csize or loc fields if they are expected to be part of the header >> >> Mach5 tiers 1-3 are clean > > Lance Andersen has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Merge branch 'master' into JDK-8314891 > - Add missing space > - Revamp isZip64ExtBlockSizeValid > - Merge branch 'master' into JDK-8314891 > - Merge branch 'master' into JDK-8314891 > - Remove tab(s) from comment > - Added additional tests, along with additional cleanup and refactoring > - Clean up some minor formatting issues > - Additional Zip64 extra header validation @LanceAndersen I noticed that this PR did not update `ZipInputStream.readLOC` to perform consistency validation between expected and actual extra field size and values. Any particular reason why processing of LOC headers was not made consistent with CEN? - PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-1802241544
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
On Wed, 8 Nov 2023 16:27:56 GMT, Eirik Bjorsnos wrote: > @LanceAndersen > > I noticed that this PR did not update `ZipInputStream.readLOC` to perform > consistency validation between expected and actual extra field size and > values. Any particular reason why processing of LOC headers was not made > consistent with CEN? Intentional, as this was a follow on to the updates which were done previously to the CEN work in August, this is follow on cleanup. Updates to ZipInputStream would be done separately under a separate PR or could be done via your work on 8303866 - PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-1802340986
