On Fri, 18 Oct 2024 12:58:17 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Please review the changes for >> [JDK-8340553](https://bugs.openjdk.org/browse/JDK-8340553), which is a >> follow-on to [JDK-8336025](https://bugs.openjdk.org/browse/JDK-8336025) >> which addresses that >> >> - ZipEntry(String) >> - ZipEntry::setComment >> - ZipEntry::setExtra >> >> currently validate that the max possiible field size is 0xFFFF(65535) >> instead of 0xFFD1(65489) not taking into account the size of the CEN header >> which is 46 bytes per the PKWare APP.NOTE 4.4.10, 4.4.11, & 4.4.12 >> >> The CSR has been approved. >> Mach5 tiers1-3 run clean as do the relevant JCK tests > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Added additional validation to ZipEntry Looks good. See comment about `headerSize` overflow plus some other minor comments. Would this change in the `ZipEntry` API description require a re-approval of the CSR, even if just a formality? src/java.base/share/classes/java/util/zip/ZipEntry.java line 106: > 104: * @throws NullPointerException if the entry name is null > 105: * @throws IllegalArgumentException if the combined length of the > entry name > 106: * and {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes. English is not my native language, but should 'CEN header size" be preceded by the definite article "the", here and in setExtra/setComment ? src/java.base/share/classes/java/util/zip/ZipEntry.java line 652: > 650: * @param comment the comment string > 651: * @throws IllegalArgumentException if the combined length > 652: * of the specified entry comment, {@linkplain #getName() entry > name}, 'entry name' is preceded by the definite article 'the' in `setExtra`, but not here. Would be good to be consistent. ------------- PR Review: https://git.openjdk.org/jdk/pull/21544#pullrequestreview-2378084998 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806487837 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806492209
