RFR: 8339711: ZipFile.Source.initCEN needlessly reads END header [v2]

[Jaikiran Pai]

On Wed, 25 Sep 2024 16:22:55 GMT, Eirik Bjørsnøs <eirbjo at 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

src/java.base/share/classes/java/util/zip/ZipFile.java line 1184:

> 1182:         private static final int[] EMPTY_META_VERSIONS = new int[0];
> 1183:         // CEN size is limited to the maximum array size in the JDK
> 1184:         private static final int MAX_CEN_SIZE = Integer.MAX_VALUE - 2;

Hello Eirik, (at least) a couple of other places in the JDK (like the `jdk.internal.util.ArraysSupport.SOFT_MAX_ARRAY_LENGTH` and `java.io.InputStream.MAX_BUFFER_SIZE`) use `Integer.MAX_VALUE - 8` as the maximum supported array size. To be consistent, we should use that same here. In fact, `jdk.internal.util.ArraysSupport.SOFT_MAX_ARRAY_LENGTH` is a public field in the JDK and we could just reference it here as follows:


// CEN size is limited to the maximum array size in the JDK
private static final int MAX_CEN_SIZE = ArraysSupport.SOFT_MAX_ARRAY_LENGTH;

test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java line 71:

> 69:     private static final int ENDOFF = ZipFile.ENDOFF; // Offset of CEN offset field within ENDHDR
> 70:     // Maximum allowed CEN size allowed by ZipFile
> 71:     private static int MAX_CEN_SIZE = Integer.MAX_VALUE - 2;

Same comment here, we should make this `Integer.MAX_VALUE - 8`

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20905#discussion_r1779484365
PR Review Comment: https://git.openjdk.org/jdk/pull/20905#discussion_r1779484503