RFR: 8341595: Clean up iteration of CEN headers in ZipFile.Source.initCEN [v2]
Eirik Bjørsnøs
eirbjo at openjdk.org
Mon Oct 7 12:16:15 UTC 2024
> Please review this PR which suggests we clean up iteration and validation logic in `ZipFile.Source::initCEN` and some related methods to use a simpler and more consistent style:
>
> * The main loop in `ZipFile.Source::initCEN` currently updates two iteration variables (`pos` and `entryPos`), where `entryPos` is always just `pos + CENHDR`. One variable should be sufficient. `entryPos` can be moved to an effectively final local variable scoped inside the loop.
> * The local variable `int limit = cen.length` no longer carries its weight and is inlined into its only use site
> * Since `entryPos` is no longer in scope for the loop predicate, this is updated to `pos <= cen.length - CENHDR`, instead of the current `entryPos <= limit`
> * The byte array passed to `countCENHeaders` contains only CEN data now, so the `size` parameter can be removed.
> * `countCENHeaders` is updated to loop on the same predicate as `initCEN`. Additionally, this method is updated to throw early if a CEN entry exceeds the CEN size (for consistency with similar logic in `checkAndAddEntry`)
> * The `headerSize` validation in `checkAndAddEntry` is updated to avoid widening conversion to `long` of a variable which can provably never exceed `Integer.MAX_VALUE` and to be consistent with `countCENHeaders`.
>
> Testing:
>
> A new test `CenSizeMaximum` is added:
> * This produces a ZIP file with a CEN size of exactly `MAX_CEN_SIZE` and verifies that the iteration logic handles ZIP files where the CEN size is on or near the limit.
> * This test also manipulates the END headers 'total number of entries' field, in order to exercise `countCENHeader`.
> * The test is a bit of a resource hog: It produces a ~1.5GB ZIP file on disk, requires > 2GB heap and takes > 15 seconds to run on my laptop. Let me know if this should be made a manual test.
>
> GHA tests are currently pending. I have run the following tests locally:
>
>
> % make test TEST="test/jdk/java/util/zip"
> % make test TEST="test/jdk/java/util/jar"
Eirik Bjørsnøs has updated the pull request incrementally with two additional commits since the last revision:
- Mark this resource hungry test manual
- For the benefit of the interpreter, extract loop predicate into a local variable
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/21378/files
- new: https://git.openjdk.org/jdk/pull/21378/files/142bc7ac..504e1022
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=21378&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=21378&range=00-01
Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/21378.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/21378/head:pull/21378
PR: https://git.openjdk.org/jdk/pull/21378
More information about the core-libs-dev
mailing list