Skip to content
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.

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

8240256: Better resource cleaning for SunPKCS11 Provider #3544

Closed
wants to merge 5 commits into from

Conversation

coffeys
Copy link
Contributor

@coffeys coffeys commented Apr 16, 2021

Added capability to allow the PKCS11 Token to be destroyed once a session is logged out from. New configuration properties via pkcs11 config file. Cleaned up the native resource poller also.

New unit test case to test behaviour. Some PKCS11 tests refactored to allow pkcs11 provider to be configured (and tested) with a config file of choice.

Reviewer request @valeriepeng


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8240256: Better resource cleaning for SunPKCS11 Provider

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3544/head:pull/3544
$ git checkout pull/3544

Update a local copy of the PR:
$ git checkout pull/3544
$ git pull https://git.openjdk.java.net/jdk pull/3544/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3544

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3544.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 16, 2021

👋 Welcome back coffeys! 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 openjdk bot added the rfr Pull request is ready for review label Apr 16, 2021
@openjdk
Copy link

openjdk bot commented Apr 16, 2021

@coffeys 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.

@openjdk openjdk bot added the security security-dev@openjdk.org label Apr 16, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Webrevs

@valeriepeng
Copy link

I will take a look. Thanks!

@seanjmullan
Copy link
Member

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Apr 22, 2021
@openjdk
Copy link

openjdk bot commented Apr 22, 2021

@seanjmullan this pull request will not be integrated until the CSR request JDK-8262226 for issue JDK-8240256 has been approved.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 26, 2021
Copy link

@valeriepeng valeriepeng left a comment

Choose a reason for hiding this comment

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

Here are some comments. Mostly just nit. Will continue looking at the test changes.
Thanks~
Valerie

@@ -411,6 +431,18 @@ private void parse() throws IOException {
if (insertionCheckInterval < 100) {
throw excLine(word + " must be at least 100 ms");
}
} else if (word.endsWith("cleaner.shortInterval")) {

Choose a reason for hiding this comment

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

Why use endsWith()? Most of other parsing code are enforcing equality, i.e. equals()?

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2011, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.

Choose a reason for hiding this comment

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

2021? Same goes for other files.

@@ -129,28 +149,12 @@ void close() {
final class SessionRef extends PhantomReference<Session>
implements Comparable<SessionRef> {

private static ReferenceQueue<Session> refQueue =
static ReferenceQueue<Session> refQueue =

Choose a reason for hiding this comment

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

nit: can now move the value assignment onto the same line. Same goes for the refQueue in P11Key.java.


static boolean drainRefQueue() {

Choose a reason for hiding this comment

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

Add a comment on this being called by the cleaner thread with the two interval configuration options? Sames goes for the one in P11Key.java.

@@ -238,6 +243,8 @@ private void closeSession(Session session) {
private final SessionManager mgr;
private final AbstractQueue<Session> pool;
private final int SESSION_MAX = 5;
// access is synchronized on 'this'

Choose a reason for hiding this comment

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

Maybe old comment?

Comment on lines 169 to 176
while (true) {
SessionKeyRef next = (SessionKeyRef) SessionKeyRef.refQueue.poll();
if (next == null) {
break;
}
found = true;
next.dispose();
}

Choose a reason for hiding this comment

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

nit: maybe this can be shortened a little with:
SessionKeyRef next;
while ((next = (SessionKeyRef) SessionKeyRef.refQueue.poll()) != null) {
found = true;
next.dispose();
}

Same goes for the other drainRefQueue() impl.

private class NativeResourceCleaner extends Thread {
private long sleepMillis = config.getResourceCleanerShortInterval();
private int count = 0;
boolean p11RefFound, SessRefFound;

Choose a reason for hiding this comment

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

nit: p11RefFound => keyRefFound?
nit: SessRefFound => sessRefFound?

Comment on lines 988 to 992
if (count > 100) {
// no reference freed for some time
// increase the sleep time
sleepMillis = config.getResourceCleanerLongInterval();
}

Choose a reason for hiding this comment

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

This if-check can be moved up to below the line "count++;"?

Comment on lines 549 to 555
String nssConfig = getNssConfig();
if (nssConfig == null) {
// issue loading libraries
return;
}
p = getSunPKCS11(nssConfig);
test.premain(p);

Choose a reason for hiding this comment

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

It seems a bit strange that the 2nd argument p isn't really used, but rather overridden with getSunPKCS11(String config) call. Do you mean to call getSunPKCS11(nssConfig, p) here if p is non-null?

Comment on lines 43 to 52

pkcs11Provider.logout();
WeakReference<SunPKCS11> weakRef = new WeakReference<>(pkcs11Provider);
pkcs11Provider = null;
for (int i = 0; i > 100; i++) {
System.gc();
Thread.sleep(100);
}
System.out.println("Finish: "+ weakRef.refersTo(null));
}

Choose a reason for hiding this comment

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

What is this particular block of code for? main(Provider) is non-static and may be called more than once, but then you store the passed-in Provider object into a static field? Also, must the static field use type SunPKCS11? It seems AuthProvider is sufficient and most of the "ap->pkcs11Provider" changes can be avoided?

@@ -0,0 +1,131 @@
/*
* Copyright (c) 2003, 2016, Oracle and/or its affiliates. All rights reserved.

Choose a reason for hiding this comment

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

New file, should just be 2021?


import jdk.test.lib.util.ForceGC;

public class MultipleLogins extends PKCS11Test {

Choose a reason for hiding this comment

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

The overall pattern seems a bit different from the typical PKCS11Test usage pattern. When a class extends PKCS11Test, the main test functionality is under its main(Provider) method. It is expected that the PKCS11Test class will call the main(Provider) method with the to-be-tested PKCS11 provider obj. Here it seems that you are doing that in the static main(String[]) method. Do you really need to extend PKCS11Test then?

@coffeys
Copy link
Contributor Author

coffeys commented May 7, 2021

Thanks for the review @valeriepeng Apologies for delay, had to finish up other tasks -- I've made all suggested edits that you made for src/ code and pushed those changes to my branch.

For the testing, then I recall the PKCS11Test framework not being able to return multiple PKCS11 Providers to me for testing (an array of providers) -- I thought editing the framework for such a use case would be useful but perhaps I'm better off just writing a customized test. I'm going to take a stab at that and will re-submit a test edit.

Apologies for including the Login.java/ Login.sh edits - they were old left over edits from testing efforts before moving on to develop the new MultipleLogins.java test.

@valeriepeng
Copy link

The update changes look good. Will wait for your test edit then.

@mlbridge
Copy link

mlbridge bot commented May 28, 2021

Mailing list message from Se=c3=a1n Coffey on security-dev:

Thanks for the review Valerie. I've gone ahead and updated the test.
You've a good point in that the PKCS11Test framework didn't suit the
test that I needed. The new test no longer extends PKCS11Test as a
result. I have kept the refactoring in PKCS11Test thought since it can
offer up some useful helper static methods. I did clean up the
testNSS(..) static methods. You're right that the 2nd Provider parameter
was redundant. Removed that.

Changes pushed.

regards,
Sean.

On 12/05/2021 19:55, Valerie Peng wrote:

Copy link

@valeriepeng valeriepeng left a comment

Choose a reason for hiding this comment

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

Changes look fine. Thanks, Valerie

@openjdk
Copy link

openjdk bot commented Jun 2, 2021

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

8240256: Better resource cleaning for SunPKCS11 Provider

Reviewed-by: valeriep

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

  • e1462e7: 8267176: StandardDoclet should provide access to Reporter and Locale
  • 56b65e4: 8267569: java.io.File.equals contains misleading Javadoc
  • 508cec7: 8267521: Post JEP 411 refactoring: maximum covering > 50K
  • 40d23a0: 8267543: Post JEP 411 refactoring: security
  • 4767758: 8267920: Create separate Events buffer for VMOperations
  • dc19bac: 8268094: Some vmTestbase/nsk tests fail after ACC_STRICT/strictfp changes
  • 2963c9e: 8268103: JNI functions incorrectly return a double after JDK-8265836
  • 6765f90: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
  • 19450b9: 8266281: Assign Symbols to the package selector expression
  • a223189: 8264774: Implementation of Foreign Function and Memory API (Incubator)
  • ... and 55 more: https://git.openjdk.java.net/jdk/compare/1d2c7ac3f7492b335757bf0fd3f6ca3941c5fc72...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 openjdk bot added the ready Pull request is ready to be integrated label Jun 2, 2021
@coffeys
Copy link
Contributor Author

coffeys commented Jun 3, 2021

/integrate

@openjdk openjdk bot closed this Jun 3, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Jun 3, 2021
@openjdk openjdk bot removed the rfr Pull request is ready for review label Jun 3, 2021
@openjdk
Copy link

openjdk bot commented Jun 3, 2021

@coffeys Since your change was applied there have been 77 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit bdeaeb4.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
3 participants