On Wed, 25 Sep 2024 16:22:55 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this cleanup PR which makes `ZipFile.Source.initCEN` not
>> include the 22-byte trailing`END` header when reading the `CEN` section of
>> the ZIP file.
>>
>> The reading of the END header was probably brought over from native code
>> with the transition to Java in JDK 9.
>>
>> In the current JDK, the END header is unused. This needlessly complicates
>> multiple code paths accessing the array since they must account for the
>> trailing END record when calculating the end of CEN position.
>>
>> Additionally, the enforcement of the maximum CEN size limit is currently off
>> by one. It allows the construction of a byte array of size
>> `Integer.MAX_VALUE - 1`, but this size is not supported by OpenJDK. Instead,
>> the maximum CEN limit should be such that is does not exceed
>> `Integer.MAX_VALUE - 2`.
>>
>> Testing:
>>
>> The `EndOfCenValidation` test is updated to test the rejection of a CEN of
>> size `Integer.MAX_VALUE - 1` as the new minumum rejected CEN size.
>>
>> The `ZipFileOpen` benchmark seems neutral to this change.
>
> Eirik Bjørsnøs 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 three additional
> commits since the last revision:
>
> - Merge branch 'master' into zipfile-endhdr
> - Merge branch 'master' into zipfile-endhdr
> - Do not include the END header when reading the CEN section
LGTM. Simplifies a bit and allows for another 22 bytes of CEN.
src/java.base/share/classes/java/util/zip/ZipFile.java line 1740:
> 1738: }
> 1739: // read in the CEN
> 1740: if (end.cenlen > MAX_CEN_SIZE) {
We'd be throwing an OOMError today if we soared too close to the limit
(`Integer.MAX_VALUE - ENDHDR - 2` and above), then throw `zerror` if we go
beyond the limit. With these changes we change to throw `zerror` exclusively.
An OOME can of course still be thrown from any subsequent allocation on a
_real_ OOM, so I'm not sure how valuable this edge case trimming is.
-------------
Marked as reviewed by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20905#pullrequestreview-2329393055
PR Review Comment: https://git.openjdk.org/jdk/pull/20905#discussion_r1775907509
