RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]

[Liam Miller-Cushon]

On Sat, 30 Mar 2024 17:28:11 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> I think that's similar idea to one of the alternatives I mentioned earlier, won't that allocate for every central directory entry? This callsite has already read the data we need into a buffer, if we end up doing something like that it might be better to do the allocation only for the call site in `validate_lochdr`, and have the shared helper take the pointer to the buffer instead of having a duplicate read.
>> 
>> It seems like there are some tradeoffs here, I can definitely restructure this, but I'd also like to get some feedback from a core-libs owner on the overall approach before iterating too much more.
>
>> I think that's similar idea to one of the alternatives I mentioned earlier, won't that allocate for every central directory entry?
> 
> Ok. Re-reading your earlier comment more closely, I see you mentioned allocating memory option.
> 
> The allocation of the buffer for reading extra fields is only done when needed (if `cen_offset` is `ZIP64_MAGICVAL` for the validate_lochdr case). I think I'm missing the part about the "allocate for every central directory entry" with the approach. Could you please elaborate a bit more?
> 
>> This callsite has already read the data we need into a buffer, if we end up doing something like that it might be better to do the allocation only for the call site in `validate_lochdr`, and have the shared helper take the pointer to the buffer instead of having a duplicate read.
>> 
>> It seems like there are some tradeoffs here, I can definitely restructure this, but I'd also like to get some feedback from a core-libs owner on the overall approach before iterating too much more.
> 
> Yeah, agreed. It's a good idea to have someone work closely with ZIP and have more insights on central directory structure to take a look at this change now.

The alternative I looked at was something like https://github.com/openjdk/jdk/commit/04e4410bb0c250ee270dda10bcf2dbd5aac90743. It avoids duplicating the loop and makes the diff very slightly shorter, but requires doing a `malloc(cenext)`. I think I was mistaken about 'allocate for every central directory entry', `validate_lochdr` should only be a small constant number of times. That still seems a little unfortunate because we don't know how big `cenext` is, but maybe that's not worth worrying about.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1545474395