On Fri, 18 Oct 2024 13:24:08 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> 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 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 ?
Not really needed but added anyways
> 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.
Done
> 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) {
fixed
> 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`, here and in `ZipOutputStream`
Changed to
`long headerSize = (long)CENHDR + name.length() + clen + elen;`
> 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;
done
> 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.comment)`
done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507539
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507648
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806509705
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806509453
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806508512
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507936
