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

[Lance Andersen]

On Fri, 18 Oct 2024 18:40:13 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Lance Andersen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address PR feedback
>
> 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.

removed

> 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

updated

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

sure done

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

done

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

sure done

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

Sure, done

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806961065
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806959754
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960501
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960667
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960010
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960336