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

[Eirik Bjørsnøs]

On Thu, 17 Oct 2024 10:44:51 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:
> 
>   Add back missing putNextEntry call

src/java.base/share/classes/java/util/zip/ZipEntry.java line 97:

> 95:     // for entries in order to not exceed 65,489 bytes minus 46 bytes for the CEN
> 96:     // header length
> 97:     private static final int MAX_NAME_COMMENT_EXTRA_SIZE =

Would it make sense to validate the actual combined length here, instead of just pretending that all other components of the combined lenghts are zero-length?

We could introduce a method like this:


/*
 * Validate that the combined length of the 46 byte CEN header, the name,
 * the extra field and the comment does not exceed 0xFFFF
 */
private void validateCombinedLength(String name, byte[] extra, String comment, String message) {
    int headerLength = ZipEntry.CENHDR;

    if (name != null) {
        headerLength += name.length();
    }
    if (extra != null) {
        headerLength += extra.length;
    }

    if (comment != null) {
        headerLength += comment.length();
    }

    if (headerLength > 0xFFFF) {
        throw new IllegalArgumentException(message);
    }
}


Then the constructor could use it like this:

validateCombinedLength(name, null, null, "entry name too long");


setExtra:


validateCombinedLength(name, extra, comment, "invalid extra field length");


setComment:

validateCombinedLength(name, extra, comment, "entry comment too long");

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1804594587