On Sat, 30 Mar 2024 17:28:11 GMT, Jiangli Zhou <jian...@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
