On Fri, 4 Apr 2025 00:39:04 GMT, Henry Jen <henry...@openjdk.org> wrote:
> This PR check the jar file to ensure entries are consistent from the central
> directory and local file header. Also check there is no duplicate entry names
> that could override the desired content by accident.
Thank you for starting the work on this Henry.
A few initial comments/improvements based on an initial pass of your PR:
- Validate that the Entry names match between the LOC and CEN including the
entry order within the headers (ZipOutputStream and most tools will write the
LOC/CEN headers in the same order)
- Warn of duplicate entries
- Check that any LOC entry exists in the CEN and any CEN entry exists in the LOC
- Be more specific in the warnings reported such as: Entry XXX found in the
LOC but not the CEN
- main.help.opt.main.validate in jar.properties should be updated to indicate
additional validation
- jar.md should also be updated for the same reason
- I would use this as an opportunity to add some comments as to what the
methods such as validate are now doing given the functions verification has
been expanded
It would also be good to validate that the MANIFEST returned ZipFile and
ZipInputStream match (this could be follow on work)
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 66:
> 64: private String mdName;
> 65: private final ZipInputStream zis;
> 66: private final Set<String> entryNames = new HashSet<>();
Please rename this to represent the CEN entries.
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 80:
> 78: }
> 79:
> 80: private void checkDuplicates(ZipEntry e) {
Please add a general comment of the purpose as this method is only used with
traversing the ZipFile and walking the CEN
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 86:
> 84: }
> 85:
> 86: private void checkZipInputStream() {
Please add a comment on the purpose of the method
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 92:
> 90: ZipEntry e;
> 91: while ((e = zis.getNextEntry()) != null) {
> 92: var entryName = e.getName();
please rename to locEntryName
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 99:
> 97: if (!entryNames.contains(entryName)) {
> 98: missingEntryNames.add(entryName);
> 99: }
It looks like you are checking to see if the LOC entry contains within the CEN
but I don't see if you are checking if the CEN entry is contained in the LOC
Another facet of validation is to compare the ordering of entries between the
LOC and CEN
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 147:
> 145: in incompatible public interfaces
> 146: warn.validator.duplicate.entry=\
> 147: Warning: More than one copy of {0} is detected
How do we know if the duplicate entry is in the CEN or LOC?
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 149:
> 147: Warning: More than one copy of {0} is detected
> 148: warn.validator.inconsistent.content=\
> 149: Warning: The list of entries does not match the content
This message could be more specific to the type of error found
test/jdk/tools/jar/MultipleManifestTest.java line 26:
> 24: /*
> 25: * @test
> 26: * @bug 8335912 8345431
I would suggesting moving the validation for multiple entries, LOC/CEN
mismatches into a separate test
-------------
PR Review: https://git.openjdk.org/jdk/pull/24430#pullrequestreview-2742819751
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028719761
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028722966
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028727832
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028730393
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028742286
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028755446
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028756250
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2028758898
