On Tue, 16 Jan 2024 13:42:30 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 534: >> >>> 532: >>> 533: long csize = get32(tmpbuf, LOCSIZ); >>> 534: long size = get32(tmpbuf, LOCLEN); >> >> Hello Eirik, I suspect this part of the change has an issue. Before reading >> the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of >> CRC, which should be read first. This now skips those 32 CRC bits and reads >> them (in the else block) after reading these sizes and that can cause >> incorrect LOC data. > > The Github actions job which runs tier1 is all successful with this proposed > change. So I'm a bit surprised that the tests didn't catch any issues, which > makes me wonder if we have enough test coverage that covers this change. > Hello Eirik, I suspect this part of the change has an issue. Before reading > the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of > CRC, which should be read first. This now skips those 32 CRC bits and reads > them (in the else block) after reading these sizes and that can cause > incorrect LOC data. How does the order of the reads from the byte array matter? Outside cache efficiency I would presume they could be read in any order? (These reads are from a temp buffer, not the stream) Could you elaborate? I'm not sure I'm following :-) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453516319
