On Wed, 8 Nov 2023 20:26:32 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
>> Please review this PR which enhances the existing CEN header validation
>> checking to ensure that the
>> size of the CEN Header + name length + comment length + extra length do not
>> exceed 65,535 bytes per the PKWare APP.NOTE 4.4.10, 4.4.11, & 4.4.12. Also
>> check that current CEN header will not exceed the length of the CEN array.
>>
>> Mach 5 tiers 1-3 are clean with this change.
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1225:
>
>> 1223: int elen = CENEXT(cen, pos);
>> 1224: int clen = CENCOM(cen, pos);
>> 1225: long headerSize = (long)CENHDR + nlen + clen + elen;
>
> Since CENHDR is 46 and nlen, clen, elen are all unsigned shorts, this sum
> cannot possibly overflow an int. Is the long conversion necessary?
>
> The specification uses the term "combined length", would it be better to name
> the local variable `combinedLength` instead?
I can remove the cast as that was a holdover. I chose to make this a long
knowing that it would not overflow but an overflow while unlikely could occur
depending on the value of `pos` in the statement below
if (headerSize > 0xFFFF || pos + headerSize > cen.length - ENDHDR) {
zerror("invalid CEN header (bad header size)");
}
I could keep headerSize an int and then move the cast to the if statement:
if (headerSize > 0xFFFF || (long)pos + headerSize > cen.length - ENDHDR) {
zerror("invalid CEN header (bad header size)");
}
I decided making headerSize a long might be clearer but do not have a strong
preference and will go with the consensus
As far as the name, I don't have a strong preference, but not sure
combinedLength is any better
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1235:
>
>> 1233:
>> 1234: if (elen > 0 && !DISABLE_ZIP64_EXTRA_VALIDATION) {
>> 1235: checkExtraFields(pos, entryPos + nlen, elen);
>
> The naming of `entryPos` confused me here. The name indicates it is the
> position where the CEN header starts, but we already have `pos` for that. (It
> actually contains the position where the encoded name starts)
>
> So perhaps it should be renamed to `namePos` or `npos` to make future
> maintenance less confusing?
>
> Also, I actually liked the `extraStartingOffset` local variable, having a
> name makes the code easier to follow than just `entryPos + nlen`. But perhaps
> `extraPos` is shorter and more consistent with other uses of `pos`?
>
> So perhaps: `long extraPos = pos + CENHDR + nlen` ?
`entryPos` was the name of the field from a previous PR so I did not see a need
to change it and decided there was no need to keep extraStartingOffset given
the change in validation above.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1388318963
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1388313503
