On Sun, 6 Oct 2024 16:42: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"
LGTM
src/java.base/share/classes/java/util/zip/ZipFile.java line 1772:
> 1770: int pos = 0;
> 1771: manifestNum = 0;
> 1772: while (pos <= cen.length - CENHDR) {
The interpreter would probably like something like:
int limit = cen.length - CENHDR;
while (pos <= limit) {
test/jdk/java/util/zip/ZipFile/CenSizeMaximum.java line 24:
> 22: */
> 23:
> 24: /* @test
Yes, this test probably should be either manual or moved to a higher tier.
-------------
Marked as reviewed by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21378#pullrequestreview-2351690639
PR Review Comment: https://git.openjdk.org/jdk/pull/21378#discussion_r1790053062
PR Review Comment: https://git.openjdk.org/jdk/pull/21378#discussion_r1790055277
