On Tue, 25 Mar 2025 20:13:16 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test with more ZipEntry in the jar
>
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1158:
>
>> 1156: if (cenEntry == null) {
>> 1157: crossChkWarnings.add(String.format(rb.getString(
>> 1158: "Entry.missing.in.JarFile.1"), entryName));
>
> Would it be more precise to change this warning to "Entry %s is present when
> reading via JarInputStream but missing when reading via JarFile"?
Updated this warning as suggested.
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1163:
>
>> 1161:
>> 1162: readEntry(jis);
>> 1163: try (InputStream cenInputStream =
>> jarFile.getInputStream(cenEntry)) {
>
> What if this returns null? Shouldn't you also emit a warning that the entry
> was read by JarInputStream but not JarFile? Also, I think `readEntry()` would
> throw an NPE.
Yes, added checking on `cenInputStream` to emit a warning if it is null.
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1241:
>
>> 1239: boolean locHasSigners = locSigners != null;
>> 1240:
>> 1241: if (cenHasSigners && locHasSigners) {
>
> So, it's ok if one entry has code signers but the other doesn't?
Fixed. Added the checking.
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1245:
>
>> 1243: List<CodeSigner> locSignerList = Arrays.asList(locSigners);
>> 1244:
>> 1245: if (!cenSignerList.equals(locSignerList)) {
>
> I think you can just call `Arrays.equals()` here.
Done.
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1247:
>
>> 1245: if (!cenSignerList.equals(locSignerList)) {
>> 1246: crossChkWarnings.add(String.format(rb.getString(
>> 1247:
>> "signature.mismatch.for.entry.1.when.comparing.jarfile.and.jarinputstream"),
>
> "Signature mismatch" is not accurate in my opinion. This is really just about
> the code signers. Can we change this warning to "Code signers are different
> for entry %s when reading from JarFile and JarInputStream".
>
> I like the words "reading from" instead of "comparing" as it seems to better
> describe what the JarFile and JarInputStream APIs for and how to diagnose the
> issue.
Updated this warning as suggested.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296602
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296685
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296741
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296799
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296907
