On Mon, 7 Oct 2024 12:16:15 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> 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 Thank you for the clean up Eirik... looks good ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21378#pullrequestreview-2352264596
