On Fri, 18 Oct 2024 14:02:09 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: > > Address PR feedback A few more nits, mainly focusing on the test for ZipEntry validation. src/java.base/share/classes/java/util/zip/ZipEntry.java line 528: > 526: * extra field data, the {@linkplain #getName() entry name}, > 527: * the {@linkplain #getComment() entry comment}, and the > 528: * {@linkplain #CENHDR CEN Header size}, exceeds 65,535 bytes. Is the comma before "exceeds" intentional? Question also applies to `setComment`. The constructor does not have a comma at this position. Suggestion: * {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes. test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 28: > 26: * @summary Verify that ZipEntry(String), ZipEntry::setComment, and > 27: * ZipEntry::setExtra throws a IllegalArgumentException when the > 28: * length of the field exceeds 65,489 bytes The summary seems slightly wrong now: Suggestion: * combined length of the CEN header fields exceeds 65,489 bytes test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 93: > 91: boolean expectException = length > MAX_NAME_COMMENT_EXTRA_SIZE; > 92: ZipEntry zipEntry = new ZipEntry(ENTRY_NAME); > 93: String comment = new String(bytes, StandardCharsets.UTF_8); Could this be just? String comment = "a".repeat(length); Applies to the name test method as well. test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 96: > 94: System.out.printf("Comment Len= %s, exception: %s%n", > comment.length(), expectException); > 95: // The comment length will trigger the IllegalArgumentException > 96: if (expectException) { Should there be an `else` here which sets the comment successfully? This applies to all three tests methods. test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 112: > 110: MAX_FIELD_LEN_MINUS_ENTRY_NAME, > 111: MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1}) > 112: void setNameLengthTest(int length) { Perhaps just 'nameLengthTest', as this does not test a setter. Suggestion: void nameLengthTest(int length) { test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 135: > 133: MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1}) > 134: void setExtraLengthTest(int length) { > 135: final byte[] bytes = new byte[length]; I think the purpose of this test may be clearer if the construction of the extra byte array was extracted as a separate method. (The exact contents seems irrelevant as longs as its well-formed) ------------- PR Review: https://git.openjdk.org/jdk/pull/21544#pullrequestreview-2378758933 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806890025 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806915381 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806900459 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806899076 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806905208 PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806902819
