On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey <coff...@openjdk.org> wrote:
>> Sufficient permissions missing if this code was ever to run with
>> SecurityManager.
>>
>> Cleanest approach appears to be use of InnocuousThread to create the
>> cleaner/poller threads.
>> Test case coverage extended to cover the SecurityManager scenario.
>>
>> Reviewer request: @valeriepeng
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move TokenPoller to Runnable
src/java.base/share/lib/security/default.policy line 131:
> 129: permission java.lang.RuntimePermission
> "accessClassInPackage.com.sun.crypto.provider";
> 130: permission java.lang.RuntimePermission
> "accessClassInPackage.jdk.internal.misc";
> 131: permission java.lang.RuntimePermission
> "accessClassInPackage.sun.security.*";
Can we just do necessary changes? I noticed that this file seems to have mixed
style, i.e. some lines are longer than 80 chars and some break into 2 lines
with length less than 80 chars. Since the whole file is mixed, maybe just do
what must be changed.
src/java.base/share/lib/security/default.policy line 142:
> 140: permission java.security.SecurityPermission
> "clearProviderProperties.*";
> 141: permission java.security.SecurityPermission
> "removeProviderProperty.*";
> 142: permission java.security.SecurityPermission
> "getProperty.auth.login.defaultCallbackHandler";
Same "avoid unnecessary changes" comment here.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
952:
> 950: AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
> 951: Thread t = InnocuousThread.newSystemThread(
> 952: "Poller " + getName(),
nit: "Poller " -> "Poller-" (like before)?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
956:
> 954: assert t.getContextClassLoader() == null;
> 955: t.setDaemon(true);
> 956: t.setPriority(Thread.MIN_PRIORITY);
nit: supply this priority value as an argument to the
InnocuousThread.newSystemThread() call instead?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
1033:
> 1031: }
> 1032: cleaner = new NativeResourceCleaner();
> 1033: AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is
un-necessary? I tried your test without it and test still passes.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
1039:
> 1037: assert t.getContextClassLoader() == null;
> 1038: t.setDaemon(true);
> 1039: t.setPriority(Thread.MIN_PRIORITY);
nit: supply this priority value as an argument to the
InnocuousThread.newSystemThread() call instead?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
1212:
> 1210:
> 1211: this.token = token;
> 1212: if (cleaner == null) {
This check seems duplicate to the one in createCleaner() call.
test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:
> 54: System.out.println("No NSS config found. Skipping.");
> 55: return;
> 56: }
Move this if-check block of code up before the for-loop?
-------------
PR: https://git.openjdk.java.net/jdk17/pull/117
