8309841: Jarsigner should print a warning if an entry is removed

[wkia]

This is backport of "8309841: Jarsigner should print a warning if an entry is removed"

Original patch does not apply cleanly to jdk8:

• some minor conflicts in JDK code
• files moved to appropriate locations
• added the check in jdk/src/share/classes/sun/security/tools/jarsigner/Main.java at line 1196

in tests:

• ed25519 algorithm was replaced with RSA in RemovedFiles.java
• JarEntry copyEntry() procedure was manually added to JarUtils.java to make JarUtilsTest working.
• the tests were adapted to Java 8, including replaced Path.of, Files.writeString, package names, and arrays processing as well.

We need this fix in jdk8, as all versions have this issue with jarsigner.

New tests successfully ran locally on Linux, x86_64.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change requires CSR request JDK-8334261 to be approved
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8309841 needs maintainer approval

Issues

• JDK-8309841: Jarsigner should print a warning if an entry is removed (Enhancement - P3 - Approved)
• JDK-8334261: Jarsigner should print a warning if an entry is removed (CSR)

Reviewers

• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/635/head:pull/635
$ git checkout pull/635

Update a local copy of the PR:
$ git checkout pull/635
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/635/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 635

View PR using the GUI difftool:
$ git pr show -t 635

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/635.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back rmarchenko! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@wkia This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8309841: Jarsigner should print a warning if an entry is removed

Reviewed-by: andrew

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@gnu-andrew) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 02: Full - Incremental (75504f7c)
• 01: Full (3c99ab3a)
• 00: Full (2adb78f3)

[gnu-andrew]

Patch mostly looks good, but I think we should backport JDK-8240235 first in its own PR. It is little more than the copyEntry method you've imported here, but also fixes other cases in JarUtils.java where it should also be used.

Also, should java.nio.file.StandardCopyOption be imported in JarUtils.java? I see the import being added but no reference to it added in the code.

[wkia]

@gnu-andrew

Patch mostly looks good, but I think we should backport JDK-8240235 first in its own PR.

Ok, if it's needed. I will do this.

Also, should java.nio.file.StandardCopyOption be imported in JarUtils.java? I see the import being added but no reference to it added in the code.

There is the line 297:

Files.move(tmpfile, jarfile, StandardCopyOption.REPLACE_EXISTING);

[wkia]

@gnu-andrew BTW JDK-8240235 modifies a line in updateJarFile procedure which was added by JDK-8211171. Do you think JDK-8211171 should be backported also?

[gnu-andrew]

@gnu-andrew

Patch mostly looks good, but I think we should backport JDK-8240235 first in its own PR.

Ok, if it's needed. I will do this.

I think it would be good to fix the existing bug in updateJar if we are bringing in the method that provides the fix anyway. Otherwise, we have different behaviour in updateJar and the new deleteEntries.

Also, should java.nio.file.StandardCopyOption be imported in JarUtils.java? I see the import being added but no reference to it added in the code.

There is the line 297:

Files.move(tmpfile, jarfile, StandardCopyOption.REPLACE_EXISTING);

Ah, thanks. I now see why I was confused here. The import is unique to 8u because 11u already has it via JDK-8211171 but the code that uses it is new to both, so the former showed up as a difference between the 8u & 11u patches, but not the latter. I should have checked the actual patch.

[gnu-andrew]

@gnu-andrew BTW JDK-8240235 modifies a line in updateJarFile procedure which was added by JDK-8211171. Do you think JDK-8211171 should be backported also?

I was just looking at the same bug with respect to where the import statement came from in 11u. I think 8211171 might be useful for 8u, but it's a bit involved to require for this change, as it updates numerous tests which use JarUtils. I'm happy for a JDK-8240235 to just update the existing method, updateJar. I'll take a look at 8211171 and include the updated line in that backport if I decide to go ahead.

[gnu-andrew]

@gnu-andrew BTW JDK-8240235 modifies a line in updateJarFile procedure which was added by JDK-8211171. Do you think JDK-8211171 should be backported also?

I was just looking at the same bug with respect to where the import statement came from in 11u. I think 8211171 might be useful for 8u, but it's a bit involved to require for this change, as it updates numerous tests which use JarUtils. I'm happy for a JDK-8240235 to just update the existing method, updateJar. I'll take a look at 8211171 and include the updated line in that backport if I decide to go ahead.

Incidentally, the main reason for 8211171 was that there were two JarUtils in 11u under jdk and jaxp. The jaxp one is not present in 8u so there's no duplication issue.

[openjdk]

@wkia this pull request can not be integrated into pr/640 due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout b8309841
git fetch https://git.openjdk.org/jdk8u-dev.git pr/640
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/640"
git push

[openjdk]

@wkia Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[gnu-andrew]

Thanks for backporting JDK-8240235 and updating this patch on top of it. The merged version looks good to go.

[openjdk]

⚠️ @wkia This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[wkia]

/approval request I'd like to backport this to 8u-dev. We need this fix in jdk8, as it has this issue with jarsigner. Original patch does not apply cleanly to jdk8, some minor conflicts resolved, files moved to appropriate locations, tests adapted. New tests successfully ran locally on Linux, x86_64.

[openjdk]

@wkia
8309841: The approval request has been created successfully.

[gnu-andrew]

/approve yes

[openjdk]

@gnu-andrew
8309841: The approval request has been approved.

[wkia]

/integrate

[openjdk]

@wkia
Your change (at version 75504f7) is now ready to be sponsored by a Committer.

[gnu-andrew]

/sponsor

[openjdk]

Going to push as commit b256b1a.

[openjdk]

@gnu-andrew @wkia Pushed as commit b256b1a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.