On Tue, 6 May 2025 18:21:30 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.
>
> Henry Jen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Restore Validator access level
Hi Henry,
Thank you for tackling this one. I am still going through this but have some
initial comments on my 1st pass
Can you also please address the following in jar.properties:
- remove the duplicate entry of `main.help.opt.extract `
- update `main.help.opt.main.validate` to document the additional actions
--validate now takes
We also need to update `open/src/jdk.jartool/share/man/jar.md` as it currently
contains no mentions of the `--validate `option
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 120:
> 118: /**
> 119: * Helper class to deduplicate entry names.
> 120: * Keep a coutner for duplicate entry names and check if the entry
> name is valid on first add.
counter -> counter
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 122:
> 120: * Keep a coutner for duplicate entry names and check if the entry
> name is valid on first add.
> 121: */
> 122: private class DedupEntryNames {
Need a better name for this class as it is hard to understand what its purpose
is from the name
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 122:
> 120: * Keep a coutner for duplicate entry names and check if the entry
> name is valid on first add.
> 121: */
> 122: private class DedupEntryNames {
This class and some of its methods I would rename as it is difficult to
understand what their purpose is and this will help future maintainers
There also needs to be more meaningful class documentation given how the
class/methods are used
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 124:
> 122: private class DedupEntryNames {
> 123: LinkedHashMap<String, Integer> entries = new LinkedHashMap<>();
> 124: boolean isCEN;
An enum might be clearer vs using a boolean for clarity
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 127:
> 125:
> 126: public DedupEntryNames(boolean isCEN) {
> 127: this.isCEN = isCEN;
Might be clearer if this was an enum vs boolean?
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 135:
> 133: isValid = false;
> 134: } else if (!isZipEntryNameValid(entryName)) {
> 135: warn(formatMsg("error.validator.bad.entry.name",
> entryName));
I think we need to consider a better message as 'malformed' might not be clear
enough (also looks like this message was added but not used previously)
error.validator.bad.entry.name=\
entry name malformed, {0}
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 135:
> 133: isValid = false;
> 134: } else if (!isZipEntryNameValid(entryName)) {
> 135: warn(formatMsg("error.validator.bad.entry.name",
> entryName));
This message for `error.validator.bad.entry.name` could be clearer as I don't
think "malformed" indicating the issues with the file name entry in the CEN/LOC
Header. We should also prefix it a "Warning:"
`entry name malformed, {0}`
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 147:
> 145: warn(formatMsg(msg, count.toString(), counter.getKey()));
> 146: }
> 147: return true;
This seems to always be true as `warn()` results in `err.println` call (unless
I am missing something)
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 150:
> 148: }
> 149:
> 150: public LinkedList<String> dedup() {
Need a better method name and a comment for the method would be helpful for
future maintainers
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 169:
> 167:
> 168: // Check next CEN entry is matching the specified name
> 169: private boolean checkNextCenEntry(String locEntryName,
> LinkedList<String> cenEntryNames) {
The method description and method name could be clearer as to its purpose
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 184:
> 182:
> 183: /*
> 184: * Retrieve entries from the XipInputStream to verify local file
> headers(LOC)
XipInputStream -> ZipInputStream
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 184:
> 182:
> 183: /*
> 184: * Retrieve entries from the XipInputStream to verify local file
> headers(LOC)
Typo: `XipInputStream`
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 215:
> 213: private boolean validate() {
> 214: try {
> 215: var dedupCEN = new DedupEntryNames(true);
Please use a more insightful variable name
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 217:
> 215: var dedupCEN = new DedupEntryNames(true);
> 216: zf.stream()
> 217: .peek(e -> dedupCEN.add(e.getName()))
I am wondering if it might be clearer to consider breaking the above into a
separate step for clarity vs part of the current stream traversal? Otherwise
there should be more comments added for future maintainers
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 147:
> 145: in incompatible public interfaces
> 146: warn.validator.duplicate.cen.entry=\
> 147: Warning: {0} copies of {1} is detected in central directory
I would change:
`Warning: {0} copies of {1} is detected in central directory`
to:
`Warning: There were {0} central directory headers found for file name {1} `
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 148:
> 146: warn.validator.duplicate.cen.entry=\
> 147: Warning: {0} copies of {1} is detected in central directory
> 148: warn.validator.duplicate.loc.entry=\
Same comment as for `warn.validator.duplicate.cen.entry`
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 150:
> 148: warn.validator.duplicate.loc.entry=\
> 149: Warning: {0} copies of {1} is detected in local file header
> 150: warn.validator.cen.only.entry=\
I would change
`Warning: Entry {0} in central directory is not in local file header`
to
`Warning: An equivalent local file header was not found for the central
directory header for the file name: {0} `
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 151:
> 149: Warning: {0} copies of {1} is detected in local file header
> 150: warn.validator.cen.only.entry=\
> 151: Warning: Entry {0} in central directory is not in local file
> header
I would change the above to something like:
Warning: An equivalent Local File header record for the Central Directory
Header record for file {0} was not found
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 152:
> 150: warn.validator.cen.only.entry=\
> 151: Warning: Entry {0} in central directory is not in local file
> header
> 152: warn.validator.loc.only.entry=\
Same comment as for `warn.validator.duplicate.loc.entry`
test/jdk/tools/jar/ValidatorTest.java line 106:
> 104: var template = out.toByteArray();
> 105: // ISO_8859_1 to keep the 8-bit value
> 106: var s = new String(template, StandardCharsets.ISO_8859_1);
Is there a reason you are not using UTF8 here given that is the charset being
used for the zip creation?
test/jdk/tools/jar/ValidatorTest.java line 145:
> 143: var template = out.toByteArray();
> 144: // ISO_8859_1 to keep the 8-bit value
> 145: var s = new String(template, StandardCharsets.ISO_8859_1);
Ok, but the zip is being created using UTF_8....
test/jdk/tools/jar/ValidatorTest.java line 232:
> 230: var err = e.getMessage();
> 231: System.out.println(err);
> 232: Assertions.assertTrue(err.contains("Warning: 2 copies of
> META-INF/MANIFEST.MF is detected in local file header"));
Given you are importing Assertions, you should be able to just use `assertTrue`
-------------
PR Review: https://git.openjdk.org/jdk/pull/24430#pullrequestreview-2819609944
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076261620
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076262727
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077860557
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076291444
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078105322
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076275933
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077910292
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078098847
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076276973
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076281636
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076283433
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077916062
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077909330
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077908477
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078106177
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078078764
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078088807
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2076319729
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078089755
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2077731161
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078159472
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2078131560
