RFR: 8340553: ZipEntry field validation does not take into account the size of a CEN header [v5]

[Eirik Bjørsnøs]

On Fri, 18 Oct 2024 14:02:09 GMT, Lance Andersen <lancea at 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