On Thu, 17 Oct 2024 11:28:33 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:
>>
>> 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:
>
>
> /* CEN header size + name length + comment length + extra length
> * should not exceed 65,535 bytes per the PKWare APP.NOTE
> * 4.4.10, 4.4.11, & 4.4.12.
> */
> private void checkCombinedLength(String name, byte[] extra, String comment,
> String message) {
>
> int elen = extra == null ? 0 : extra.length;
> int clen = comment == null ? 0 : comment.length();
>
> long headerLength = ZipEntry.CENHDR + name.length() + elen + clen;
>
> if (headerLength > 0xFFFF) {
> throw new IllegalArgumentException(message);
> }
> }
>
>
>
> Then the constructor could use it like this:
>
> checkCombinedLength(name, null, null, "entry name too long");
>
>
> setExtra:
>
>
> checkCombinedLength(name, extra, comment, "invalid extra field length");
>
>
> setComment:
>
> checkCombinedLength(name, extra, comment, "entry comment too long");
>
>
>
> With this solution, I suppose`ZipEntry` enforces the invariant fully, and
> repeating the validation in `ZipOutputStream` should be unneccessary.
>
> Or did I miss something?
I had thought about that and decided to keep the changes as they are. I am not
opposed to revisiting this in a follow on PR. Any additional changes would
require more javadoc updates to address the overall change in validation.
So after we fork JDK 24, happyt to revisit.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1804839093
