Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c869d47
8366182: Some PKCS11Tests are being skipped when they shouldn't
mpdonova Sep 2, 2025
cd250fa
refactored to encapsulate the version as a record instead of doing re…
mpdonova Sep 5, 2025
da7faa4
Merge branch 'master' into nss-skipped
mpdonova Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions test/jdk/sun/security/pkcs11/Cipher/TestKATForGCM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,12 @@ public void main(Provider p) throws Exception {
System.out.println("Exception occured using " + p.getName() + " version " + p.getVersionStr());

if (isNSS(p)) {
double ver = getNSSInfo("nss");
String ver = getNSSInfo("nss");
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

double minor = Double.parseDouble(ver.substring(idx+1));
if (major == 3 && minor > 13.9 && minor < 15 && osName.equals("Linux")) {
// warn about buggy behaviour on Linux with nss 3.14
System.out.println("Warning: old NSS " + ver + " might be problematic, consider upgrading it");
}
Expand Down
17 changes: 10 additions & 7 deletions test/jdk/sun/security/pkcs11/KeyStore/SecretKeysBasic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2008, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2008, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -116,11 +116,14 @@ private static boolean checkSecretKeyEntry(String alias,
// A bug in NSS 3.12 (Mozilla bug 471665) causes AES key lengths
// to be read incorrectly. Checking for improper 16 byte length
// in key string.
if (isNSS(provider) && expected.getAlgorithm().equals("AES") &&
(getNSSVersion() >= 3.12 && getNSSVersion() <= 3.122)) {
System.out.println("NSS 3.12 bug returns incorrect AES key "+
"length breaking key storage. Aborting...");
return true;
if (isNSS(provider) && expected.getAlgorithm().equals("AES")) {
String version = getNSSVersion();
if (version.equals("3.12") || version.equals("3.12.1")
|| version.equals("3.12.2")) {
System.out.println("NSS 3.12 bug returns incorrect AES key " +
"length breaking key storage. Aborting...");
return true;
}
}

if (saveBeforeCheck) {
Expand Down Expand Up @@ -168,7 +171,7 @@ private static void dumpKey(String info, SecretKey key) {
private static void doTest() throws Exception {
// Make sure both NSS libraries are the same version.
if (isNSS(provider) &&
(getLibsoftokn3Version() != getLibnss3Version())) {
(!getLibsoftokn3Version().equals(getLibnss3Version()))) {
System.out.println("libsoftokn3 and libnss3 versions do not match. Aborting test...");
return;
}
Expand Down
75 changes: 34 additions & 41 deletions test/jdk/sun/security/pkcs11/PKCS11Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -83,7 +85,7 @@ public abstract class PKCS11Test {
private static final String NSS_BUNDLE_VERSION = "3.111";
private static final String NSSLIB = "jpg.tests.jdk.nsslib";

static double nss_version = -1;
static String nss_version = null;
static ECCState nss_ecc_status = ECCState.Basic;

// The NSS library we need to search for in getNSSLibDir()
Expand All @@ -93,8 +95,8 @@ public abstract class PKCS11Test {

// NSS versions of each library. It is simpler to keep nss_version
// for quick checking for generic testing than many if-else statements.
static double softoken3_version = -1;
static double nss3_version = -1;
static String softoken3_version = null;
static String nss3_version = null;
static Provider pkcs11 = newPKCS11Provider();
private static String PKCS11_BASE;
private static Map<String, String[]> osMap;
Expand Down Expand Up @@ -258,13 +260,23 @@ private static String getOsId() {
}

static boolean isBadNSSVersion(Provider p) {
double nssVersion = getNSSVersion();
if (isNSS(p) && nssVersion >= 3.11 && nssVersion < 3.12) {
System.out.println("NSS 3.11 has a DER issue that recent " +
"version do not, skipping");
return true;
String nssVersion = getNSSVersion();
if (isNSS(p)) {
// bad version is just between [3.11,3.12)
// strings may include a trailing dash e.g., 3.111-debug
Pattern pattern = Pattern.compile("^(\\d+)(?:\\.(.*?))?(?:-.*)?$");
Matcher m = pattern.matcher(nssVersion);
if (m.matches()) {
int major = Integer.parseInt(m.group(1));
String tmp = m.group(2);
double minor = tmp == null ? 0 : Double.parseDouble(tmp);
return major == 3 && (11 <= minor && minor < 12);
} else {
throw new RuntimeException("Unexpected version format: " + nssVersion);
}
} else {
return false;
}
return false;
}

protected static void safeReload(String lib) {
Expand Down Expand Up @@ -293,26 +305,26 @@ public static boolean isNSS(Provider p) {
return p.getName().equalsIgnoreCase("SUNPKCS11-NSS");
}

static double getNSSVersion() {
if (nss_version == -1)
static String getNSSVersion() {
if (nss_version == null)
getNSSInfo();
return nss_version;
}

static ECCState getNSSECC() {
if (nss_version == -1)
if (nss_version == null)
getNSSInfo();
return nss_ecc_status;
}

public static double getLibsoftokn3Version() {
if (softoken3_version == -1)
public static String getLibsoftokn3Version() {
if (softoken3_version == null)
return getNSSInfo("softokn3");
return softoken3_version;
}

public static double getLibnss3Version() {
if (nss3_version == -1)
public static String getLibnss3Version() {
if (nss3_version == null)
return getNSSInfo("nss3");
return nss3_version;
}
Expand All @@ -327,7 +339,7 @@ static void getNSSInfo() {
// $Header: NSS <version>
// Version: NSS <version>
// Here, <version> stands for NSS version.
static double getNSSInfo(String library) {
static String getNSSInfo(String library) {
// look for two types of headers in NSS libraries
String nssHeader1 = "$Header: NSS";
String nssHeader2 = "Version: NSS";
Expand All @@ -336,15 +348,15 @@ static double getNSSInfo(String library) {
int i = 0;
Path libfile = null;

if (library.compareTo("softokn3") == 0 && softoken3_version > -1)
if (library.compareTo("softokn3") == 0 && softoken3_version != null)
return softoken3_version;
if (library.compareTo("nss3") == 0 && nss3_version > -1)
if (library.compareTo("nss3") == 0 && nss3_version != null)
return nss3_version;

try {
libfile = getNSSLibPath();
if (libfile == null) {
return 0.0;
return "0.0";
}
try (InputStream is = Files.newInputStream(libfile)) {
byte[] data = new byte[1000];
Expand Down Expand Up @@ -380,7 +392,7 @@ static double getNSSInfo(String library) {
if (!found) {
System.out.println("lib" + library +
" version not found, set to 0.0: " + libfile);
nss_version = 0.0;
nss_version = "0.0";
return nss_version;
}

Expand All @@ -393,26 +405,7 @@ static double getNSSInfo(String library) {
version.append(c);
}

// If a "dot dot" release, strip the extra dots for double parsing
String[] dot = version.toString().split("\\.");
if (dot.length > 2) {
version = new StringBuilder(dot[0] + "." + dot[1]);
for (int j = 2; dot.length > j; j++) {
version.append(dot[j]);
}
}

// Convert to double for easier version value checking
try {
nss_version = Double.parseDouble(version.toString());
} catch (NumberFormatException e) {
System.out.println("===== Content start =====");
System.out.println(s);
System.out.println("===== Content end =====");
System.out.println("Failed to parse lib" + library +
" version. Set to 0.0");
e.printStackTrace();
}
nss_version = version.toString();

System.out.print("library: " + library + ", version: " + version + ". ");

Expand Down
12 changes: 7 additions & 5 deletions test/jdk/sun/security/pkcs11/Secmod/AddTrustedCert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -121,10 +121,12 @@ public static void main(String[] args) throws Exception {
}

private static boolean improperNSSVersion(Provider p) {
double nssVersion = getNSSVersion();
if (p.getName().equalsIgnoreCase("SunPKCS11-NSSKeyStore")
&& nssVersion >= 3.28 && nssVersion < 3.35) {
return true;
String nssVersion = getNSSVersion();
if (p.getName().equalsIgnoreCase("SunPKCS11-NSSKeyStore")) {
int idx = nssVersion.indexOf(".");
double major = Double.parseDouble(nssVersion.substring(0, idx));
double minor = Double.parseDouble(nssVersion.substring(idx+1));
return major == 3 && (minor >= 28 && minor < 35);
}

return false;
Expand Down
21 changes: 13 additions & 8 deletions test/jdk/sun/security/pkcs11/Signature/TestDSAKeyLength.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -47,13 +47,18 @@ public static void main(String[] args) throws Exception {

@Override
protected boolean skipTest(Provider provider) {
double version = getNSSVersion();
String[] versionStrs = Double.toString(version).split("\\.");
int major = Integer.parseInt(versionStrs[0]);
int minor = Integer.parseInt(versionStrs[1]);
if (isNSS(provider) && (version == 0.0 || (major >= 3 && minor >= 14))) {
System.out.println("Skip testing NSS " + version);
return true;
if (isNSS(provider)) {
String version = getNSSVersion();
if (version == null) {
return true;
}
int idx = version.indexOf(".");
double major = Double.parseDouble(version.substring(0, idx));
double minor = Double.parseDouble(version.substring(idx+1));
if (major >= 3 && minor >= 14){
System.out.println("Skip testing NSS " + version);
return true;
}
}

return false;
Expand Down
1 change: 1 addition & 0 deletions test/jdk/sun/security/pkcs11/ec/TestECDH.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ protected boolean skipTest(Provider p) {
* PKCS11Test.main will remove this provider if needed
*/
Providers.setAt(p, 1);
System.out.println("Testing provider " + p.getName());

if (false) {
KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC", p);
Expand Down