On Thu, 15 May 2025 21:57:17 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:
>
> Adjust message based on review feedback
Hi Henry,
A few comments based on an initial pass through last update
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 65:
> 63: * - does not contain a backslash, '\'
> 64: * - does not contain a drive letter
> 65: * - path element does not include '..'
change to
> * - path element does not include '.' or '..'
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 117:
> 115: * and UNIX file systems etc.
> 116: * Also validate that the file name is not "." and that any name
> element is
> 117: * not equal to ".."
Also validate that the file name is not "." or ".." and that any name element is
* not equal to ".." or "."
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 132:
> 130: * - CEN and LOC should have same entries, in the same order
> 131: * NOTE: This implementation assumes CEN entries are to be added
> before
> 132: * add any LOC entries.
I think you should probably expand this comment a bit with the expected work
flow as it takes a while to grasp your intent otherwise
src/jdk.jartool/share/classes/sun/tools/jar/Validator.java line 155:
> 153: }
> 154:
> 155: boolean isPlaceHolder() {
A comment would be helpful here for future maintainers
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 287:
> 285: \ or invalid file names that could lead to
> unintended effects.\n\
> 286: \ Check with the developer to ensure the jar
> archive integrity\n\
> 287: \ when warnings observed after using this
> option.
I think this could be improved perhaps something like:
main.help.opt.main.validate=\
\ --validate Validate the contents of the jar archive. This
option will:\n\
\ + validate that the API exported by a
multi-release\n\
\ jar archive is consistent across all
different release\n\
\ versions.\n\
\ + warn if there are duplicate or invalid file
names\n\
I think you can remove the verbiage WRT "Check with..." from the help message
src/jdk.jartool/share/man/jar.md line 111:
> 109: `--validate`
> 110: : Validate the contents of the jar archive.
> 111: Check with the developer to ensure the jar archive integrity
You can remove here and add a suggestion in the "integrity of a Jar Archive"
section as to what to do.
src/jdk.jartool/share/man/jar.md line 223:
> 221:
> 222: ## Integrity of a jar Archive
> 223: As a jar archive is based on ZIP format, it is possible to manufacture a
> jar archive using tools
manufacture -> create
src/jdk.jartool/share/man/jar.md line 224:
> 222: ## Integrity of a jar Archive
> 223: As a jar archive is based on ZIP format, it is possible to manufacture a
> jar archive using tools
> 224: other than the `jar` command. The `--validate` options checks a jar
> archive for some integrity
`The '--validate' option performs the following integrity checks:`
src/jdk.jartool/share/man/jar.md line 237:
> 235: - The API exported by a multi-release jar archive is consistent across
> all different release
> 236: versions.
> 237:
I would consider something similar to:
- That there are no duplicate Zip Entry file names
- Verify that the Zip Entry file name:
- is not an absolute path
- the file name is not '.' or '..'
- does not contain a backslash, ''
- does not contain a drive letter
- path element does not include '.' or '..
- The API exported by a multi-release jar archive is consistent across all
different release versions.
The jar tool will return a status code of 0 if there were no integrity issues
encountered and a status code of 1 an issue was found.
When an integrity issue is reported, it will often require that the jar file is
re-created by the original source of the jar file
```.
test/jdk/tools/jar/ValidatorTest.java line 325:
> 323: }
> 324:
> 325: private void rm(String cmdline) {
Maybe I missed it, but is this method used?
-------------
PR Review: https://git.openjdk.org/jdk/pull/24430#pullrequestreview-2847026334
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093296698
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093300176
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093421481
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093422620
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093445110
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093448844
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093450314
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093452262
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093475451
PR Review Comment: https://git.openjdk.org/jdk/pull/24430#discussion_r2093537089
