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

[Eirik Bjørsnøs]

On Fri, 18 Oct 2024 12:58:17 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:
> 
>   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