On Mon, 14 Aug 2023 17:44:06 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/zip/ZipFile.java line 1364: >> >>> 1362: * As the fields must appear in order, the block size >>> indicates which >>> 1363: * fields to expect: >>> 1364: * 0 - May be written out by Ant and Apache Commons >>> Compress Library >> >> I don't like that `isZip64ExtBlockSizeValid()` still accepts `0` as *valid* >> input. I think we should fully handle the zero case in >> `checkZip64ExtraFieldValues()` (also see my comments there). > > Hi Volker, > > I understand your point and I had done that previously but decided I did > not like the flow of the code that way which is why I moved the check. I > prefer to leave it as is. I don't think this is a question of "taste" because `isZip64ExtBlockSizeValid()` suggests that the method will check for *valid* sizes and to my understanding `0` is not a valid input. This method might also be called from other places in the future which do not handle the zero case appropriately. In any case, I'm ready to accept this as a case of "Disagree and Commit" :) but in that case please update at least the comment below to something like "*..Note we do not need to check blockSize is >= 8 as we know its length is at least 8 by now*" because "*..from the call to isZip64ExtBlockSizeValid()*" isn't true any more. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293994175
