8366182: Some PKCS11Tests are being skipped when they shouldn't #27095

mpdonova · 2025-09-04T13:59:22Z

This PR updates PKCS11 tests to better handle NSS version numbers. The previous code treated the version numbers as double values and used comparison operators. The problem is that it incorrectly treats 3.111 as between 3.11 and 3.12. This update parses and compares the major and minor version numbers separately.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8366182: Some PKCS11Tests are being skipped when they shouldn't (Bug - P4)

Reviewers

Rajan Halade (@rhalade - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27095/head:pull/27095
$ git checkout pull/27095

Update a local copy of the PR:
$ git checkout pull/27095
$ git pull https://git.openjdk.org/jdk.git pull/27095/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27095

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27095.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-04T14:00:04Z

👋 Welcome back mdonovan! 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 · 2025-09-04T14:00:23Z

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

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

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

8366182: Some PKCS11Tests are being skipped when they shouldn't

Reviewed-by: rhalade

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 571 new commits pushed to the master branch:

b0721e2: 8368982: Test sun/security/tools/jarsigner/EC.java completed and timed out
a0e0b2d: 8370057: Correct scale handling of BigDecimal.sqrt
869112e: 8026776: Broken API names in API doc
... and 568 more: https://git.openjdk.org/jdk/compare/7e738f0d906e574706a277fabbc2cc1df6f11f19...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-09-04T14:01:01Z

@mpdonova The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-09-04T14:04:25Z

Webrevs

myankelev · 2025-09-04T15:37:57Z

test/jdk/sun/security/pkcs11/Cipher/TestKATForGCM.java

                String osName = System.getProperty("os.name");
-                if (ver > 3.139 && ver < 3.15 && osName.equals("Linux")) {
+                int idx = ver.indexOf(".");
+                double major = Double.parseDouble(ver.substring(0, idx));


IMO the split between major version and minor is a bit hard to read. Wouldn't it be easier to just get a major.minor version entirely with something like:

String[] splitParts = ver.split("//."); Double.parseDouble(splitParts.length > 1 ? splitParts[0] + "." + splitParts[1] : splitParts[0]);

This way it will always take a full double value (1.13 will not be the same as 1.1, as it's now as far as I can see) and would be a bit easier to understand

The checking for only major version could be either doubleVersion<4 && doubleVersion>=3 or even cleaner, using floor function Math.floor(doubleVersion)

And the same for the other files.

What do you think?

I may be misunderstanding your comment but it looks like you're using a single, double value to compare versions. The original code did that but it doesn't work in all cases. The problem occurs when checking a range such as

version >= 3.11 && version < 3.12

If the version number is 3.111, then the comparison is true and tests are skipped, even though version 3.111 is "greater" than 3.12. So this code creates two doubles: major and minor to do separate comparisons of the values.

To further complicate things, NSS also has versions of the form x.y.z. The original code combined the 'y' and 'z' components to go from 3.11.9 to 3.119. My change would create major version 3.0 and minor version 11.9.

Ah, you're right, I didn't think of this case! This approach will be easier then, I agree.

Still, ver.substring(idx+1) may cause errors in cases like this NSS 3.15.3.1 release notes. It will try to convert 15.3.1 to a double. Do you think changing to something like this would be worth it?

String verSubstring = ver.substring(idx+1); String[] splitParts = verSubstring.split("//."); Double minor = Double.parseDouble(splitParts.length > 1 ? splitParts[0] + "." + splitParts[1] : splitParts[0]);

That's a good point about 4-component version numbers. Frankly, I was hoping that NSS never did that. I refactored the version parsing code to create a Version record with major, minor, and patch versions. This way, individual tests don't have to do the String parsing. If we end up with a test that has to be skipped due to that 4th component, we can update the parsing accordingly. Alternately, I can add it since I'm here now... what is that fourth component called?

Thank you for the changes!
I'm not sure if it's even needed to be honest, it would be an overkill imo. I can't find the exact naming on the nss website, but I think the common one would be major.minor.patch.hotfix

…peated string parsing

bridgekeeper · 2025-10-15T12:15:00Z

@mpdonova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mpdonova · 2025-10-24T11:09:43Z

/integrate

openjdk · 2025-10-24T11:11:02Z

Going to push as commit cc9483b.
Since your change was applied there have been 581 commits pushed to the master branch:

470eedb: 8370511: test/jdk/javax/swing/JSlider/bug4382876.java does not release previously pressed keys
f73e56e: 8361894: sun/security/krb5/config/native/TestDynamicStore.java ensure that the test is run with sudo
b7a4c9c: 8366240: Improve memory ordering in new CPU Time Profiler
... and 578 more: https://git.openjdk.org/jdk/compare/7e738f0d906e574706a277fabbc2cc1df6f11f19...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-24T11:11:11Z

@mpdonova Pushed as commit cc9483b.

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

8366182: Some PKCS11Tests are being skipped when they shouldn't

c869d47

openjdk bot added security [email protected] rfr Pull request is ready for review labels Sep 4, 2025

myankelev reviewed Sep 4, 2025

View reviewed changes

mpdonova added 2 commits September 5, 2025 09:40

refactored to encapsulate the version as a record instead of doing re…

cd250fa

…peated string parsing

Merge branch 'master' into nss-skipped

da7faa4

rhalade approved these changes Oct 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 23, 2025

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2025

openjdk bot closed this Oct 24, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 24, 2025

8366182: Some PKCS11Tests are being skipped when they shouldn't #27095

8366182: Some PKCS11Tests are being skipped when they shouldn't #27095

Uh oh!

Conversation

mpdonova commented Sep 4, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 4, 2025

Uh oh!

openjdk bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

myankelev Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

mpdonova Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

mpdonova Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Oct 15, 2025

Uh oh!

mpdonova commented Oct 24, 2025

Uh oh!

openjdk bot commented Oct 24, 2025

Uh oh!

openjdk bot commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mpdonova commented Sep 4, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 4, 2025 •

edited

Loading

openjdk bot commented Sep 4, 2025 •

edited

Loading

mlbridge bot commented Sep 4, 2025 •

edited

Loading