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 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
