On Tue, 29 Sep 2026 17:20:10 GMT, Alan Bateman <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - include invalid/corrupt word in the manifest parsing message >> - remove ternary operator >> - merge latest from master branch >> - merge latest from master branch >> - remove incorrect comment about localization of error messages >> - update test >> - 8392966: Improve error message for executable JAR file > > 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? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33064#discussion_r4144875973
