8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 #18588

valeriepeng · 2024-04-03T01:11:47Z

This PR fixes a problem regarding the usage of dlerror() where an earlier error message causes a premature error out. Added extra code to clear out earlier error message and made minor code refactoring.

No regression test as this can't be reproduced using the NSS library from artifactory and thus the noreg-hard label.

Thanks!

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-8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 (Bug - P3)

Reviewers

Daniel Jeliński (@djelinski - Reviewer)
Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18588

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

Using diff file

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

Webrev

Link to Webrev Comment

…erface prior to V3.0

bridgekeeper · 2024-04-03T01:12:32Z

👋 Welcome back valeriep! 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 · 2024-04-03T01:13:28Z

@valeriepeng 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:

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0

Reviewed-by: djelinski, weijun

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

fbc1e66: 8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32
a887fd2: 8316991: Reduce nullable allocation merges
e702646: 8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails
7e5ef79: 8323116: [REDO] Computational test more than 2x slower when AVX instructions are used
9467720: 8329875: Serial: Move preservedMarks.inline.hpp to serialFullGC.cpp
a4dd2e9: 8329766: Serial: Refactor SerialBlockOffsetTable API
212a253: 8329623: NegativeArraySizeException encoding large String to UTF-8
dd930c5: 8329787: Fix typo in CLDRConverter
115f419: 8329659: Serial: Extract allowed_dead_ratio from ContiguousSpace
9ac3b77: 8329775: Serial: Remove unused declarations in serialFullGC.hpp
... and 127 more: https://git.openjdk.org/jdk/compare/892b8bb6d1725119e4c9ada8f2617c15f8af674e...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 · 2024-04-03T01:13:48Z

@valeriepeng 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 · 2024-04-03T01:16:19Z

Webrevs

djelinski · 2024-04-03T08:39:32Z

src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c

        C_GetInterface = (CK_C_GetInterface) dlsym(hModule, "C_GetInterface");
-        if ((C_GetInterface != NULL) && (dlerror() == NULL)) {
+        if (C_GetInterface != NULL) {
            TRACE0("Connect: Found C_GetInterface func\n");
            rv = (C_GetInterface)(NULL, NULL, &interface, 0L);
            if (ckAssertReturnValueOK(env, rv) == CK_ASSERT_OK) {
                goto setModuleData;
            }


Do we need an else goto cleanup here?

Not really, the intention is to continue with the C_GetFunctionList (or the method named by "getFunctionListStr").

if that's the intention, then you shouldn't be using ckAssertReturnValueOK, which throws an exception if rv is anything other than CK_ASSERT_OK

Good point, I will change it.

djelinski · 2024-04-03T08:49:01Z

src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c

@@ -91,7 +91,7 @@ JNIEXPORT jobject JNICALL Java_sun_security_pkcs11_wrapper_PKCS11_connect
    jobject globalPKCS11ImplementationReference;
    char *systemErrorMessage;
    char *exceptionMessage;
-    const char *getFunctionListStr = NULL;
+    const char *getFunctionListStr = "C_GetFunctionList";


If this value ever gets used by ReleaseStringUTFChars, the failure will be spectacular 🍿

We do have checks for jGetFunctionList != NULL before calling ReleaseStringUTFChars() with it. So, this shouldn't be an issue?

this will be an issue if jGetFunctionList != NULL and dlopen fails, resulting in goto cleanup before getFunctionListStr is reassigned.

Hmm, ok, I will change it. Thanks~

djelinski

LGTM. One nit, feel free to ignore.

djelinski · 2024-04-04T06:23:00Z

src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c

+    dlerror(); // clear any old error message not fetched
+    C_GetFunctionList = (CK_C_GetFunctionList) dlsym(hModule,
+            getFunctionListStr);
+    if ((systemErrorMessage = dlerror()) != NULL){


Would it make sense to move this check under if (C_GetFunctionList == NULL)? If C_GetFunctionList is non-null, do we care about the value of dlerror?

Yes, makes sense, I'll change it.

valeriepeng · 2024-04-06T00:11:29Z

@djelinski Thanks much for the review!
I'll integrate once I got one more reviewer from my team.

wangweij · 2024-04-08T17:38:51Z

src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c

-        TRACE0("Connect: Found C_GetFunctionList func\n");
+        TRACE1("Connect: No %s func\n", getFunctionListStr);
+        p11ThrowIOException(env, "ERROR: C_GetFunctionList == NULL");
+        goto cleanup;


Just a small comment. The dlerror is only for debugging purpose but the code above looks like it is used to determine whether to goto cleanup.

How about this:

if (C_GetFunctionList == NULL) { if ((systemErrorMessage = dlerror()) != NULL){ p11ThrowIOException(env, systemErrorMessage); } else { TRACE1("Connect: No %s func\n", getFunctionListStr); p11ThrowIOException(env, "ERROR: C_GetFunctionList == NULL"); } goto cleanup; }

Also, why is TRACE1 not called when dlerror is not NULL?

Sure, thanks for the suggestion. I'll change and re-run the tests.

valeriepeng · 2024-04-09T21:50:51Z

/integrate

openjdk · 2024-04-09T21:51:52Z

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

316361b: 8328318: Wrong description in X509Extension.getExtensionValue method javadoc
b80ba08: 8329967: Build failure after JDK-8329628
4bba445: 8325659: Normalize Random usage by incubator vector tests
6736792: 8329628: Additional changes after JDK-8329332
1e02a13: 8328614: hsdis: dlsym can't find decode symbol
23d161d: 8328630: Add logging when needed symbols in dll are missing.
2e925f2: 8329726: Use non-short forward jumps in lightweight locking
e75e1cb: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
f9bc2db: 8325371: Missing ClassFile.Option in package summary
a8fbeec: 8329956: G1: Remove unimplemented collection_set_candidate_short_type_str
... and 153 more: https://git.openjdk.org/jdk/compare/892b8bb6d1725119e4c9ada8f2617c15f8af674e...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-09T21:51:59Z

@valeriepeng Pushed as commit 6276789.

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

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 int…

66682f9

…erface prior to V3.0

openjdk bot added the rfr Pull request is ready for review label Apr 3, 2024

openjdk bot added the security security-dev@openjdk.org label Apr 3, 2024

djelinski reviewed Apr 3, 2024

View reviewed changes

Address review comments

15729fa

djelinski approved these changes Apr 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 4, 2024

Address one more review comment.

65daabf

djelinski approved these changes Apr 5, 2024

View reviewed changes

wangweij reviewed Apr 8, 2024

View reviewed changes

Address review comment from Weijun

a2d9a5f

wangweij approved these changes Apr 9, 2024

View reviewed changes

djelinski approved these changes Apr 9, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2024

openjdk bot closed this Apr 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 9, 2024

valeriepeng deleted the JDK-8328785 branch April 9, 2024 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 #18588

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 #18588

valeriepeng commented Apr 3, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 3, 2024

openjdk bot commented Apr 3, 2024 •

edited

Loading

openjdk bot commented Apr 3, 2024

mlbridge bot commented Apr 3, 2024 •

edited

Loading

djelinski Apr 3, 2024

valeriepeng Apr 3, 2024

djelinski Apr 3, 2024

valeriepeng Apr 3, 2024

djelinski Apr 3, 2024

valeriepeng Apr 3, 2024

djelinski Apr 3, 2024

valeriepeng Apr 3, 2024

djelinski left a comment

djelinski Apr 4, 2024

valeriepeng Apr 4, 2024

valeriepeng commented Apr 6, 2024

wangweij Apr 8, 2024

valeriepeng Apr 8, 2024

valeriepeng commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 #18588

8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0 #18588

Conversation

valeriepeng commented Apr 3, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 3, 2024

openjdk bot commented Apr 3, 2024 • edited Loading

openjdk bot commented Apr 3, 2024

mlbridge bot commented Apr 3, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djelinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriepeng commented Apr 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriepeng commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

valeriepeng commented Apr 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 3, 2024 •

edited

Loading

mlbridge bot commented Apr 3, 2024 •

edited

Loading