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 src/java.base/share/classes/java/util/zip/ZipEntry.java line 723: > 721: * @return true if valid CEN Header size; false otherwise > 722: */ > 723: private static boolean isCENHeaderValid(String name, byte[] extra, > String comment) { Extra space before `boolean` Suggestion: private static boolean isCENHeaderValid(String name, byte[] extra, String comment) { src/java.base/share/classes/java/util/zip/ZipEntry.java line 726: > 724: int clen = comment == null ? 0 : comment.length(); > 725: int elen = extra == null ? 0 : extra.length; > 726: int headerSize = CENHDR + name.length() + clen + elen; `headerSize` may overflow `int` src/java.base/share/classes/java/util/zip/ZipEntry.java line 727: > 725: int elen = extra == null ? 0 : extra.length; > 726: int headerSize = CENHDR + name.length() + clen + elen; > 727: return headerSize <= MAX_COMBINED_CEN_HEADER_SIZE ? true : false; Unless this was a stylistic choice, this might be simpler: Suggestion: return headerSize <= MAX_COMBINED_CEN_HEADER_SIZE; src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 268: > 266: // should not exceed 65,535 bytes per the PKWare APP.NOTE > 267: // 4.4.10, 4.4.11, & 4.4.12. > 268: int clen = e.comment == null ? 0 : e.comment.length(); If `ZipEntry.isCENHeaderValid` was package-protected, it seems you could use it from here, like: `ZipEntry.isCENHeaderValid(e.name, e.extra, e.length)` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806470587 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806472325 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806472519 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806476434
