On Wed, 30 Sep 2026 13:04:59 GMT, Jaikiran Pai <[email protected]> wrote:
>> src/java.base/share/native/libjli/parse_manifest.c line 439: >> >>> 437: } >>> 438: free(buffer); >>> 439: return -2; /* entry not found in the ZIP/JAR */ >> >> The comment on the loop has "Note that a valid zip/jar must have an ENDHDR >> (with ENDSIG) after the Central Directory". Is the existing code missing a >> ENDSIG_AT to decide if it should return -1 or 2 ? > > Hello Alan, I had a more detailed look and ran some experiments. The presence > of the end-of-central-dir signature (the `ENDSIG`) is already guaranteed > (through the `find_positions(...)` call) even before we start looping over > the central directory entries in this `find_file()` function. If the `ENDSIG` > is missing in the ZIP, then `find_file()` will return `-1` even before > starting the while loop, and thus the ZIP will be considered invalid/corrupt. > So on that front, this implementation is fine. > > The second aspect of this is that when the central directory entries end, and > we break out of the while loop, then the comment says that the ZIP file must > contain the `ENDSIG` after the central directory entries end. However it > doesn't say that `ENDSIG` must immediately start at the next byte. I went > back and ran some experiments and although the `ENDSIG` is expected to > immediately follow the last central directory entry, the ZIP structure itself > is flexible to allow for additional (arbitrary) bytes between the last > central directory entry and the `ENDSIG`. In fact, some prominent ZIP tools > allow for such ZIP files to be functional (you can list and extract files > from such ZIP files). They do print out a message about this oddity when > working on those files. > > Given this, I think adding a check for `ENDSIG` and returning `-1` if that > check fails would end up considering such ZIP files as corrupt/invalid and > that may not be ideal. I can adjust the comment at the start of the while > loop to be a bit more clear if that helps. > > Do you think we should add the tighter check here? I looked at it again and I think you are right, it's the comment that put me off and maybe we should clarify it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33064#discussion_r4158627951
