<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>Hi Valerie, <br>
</p>
<p>many thanks for the thorough review. I've taken all your feedback
on board with the latest push. Some of the test anomalies were a
result of previous iterations of test edits I had been making.</p>
<p>Regarding the extra edits in
"src/java.base/share/lib/security/default.policy", I had assumed
it would be ok to tidy up the module under edit but I've reverted
the unrelated changes now.</p>
<p>I was doubtful about removing the AccessController.doPrivileged
blocks around the InnocuousThread.newSystemThread calls given that
I wasn't sure which path the calling code would come from but on
re-examination, I think it's ok to remove. The module provides the
necessary permissions already and use of InnocuousThread solves
the original permissions issue. Thanks for spotting!</p>
<p>
<blockquote type="cite">
<p>In <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk17/pull/117*discussion_r659082598__;Iw!!ACWV5N9M2RV99hQ!d12Warg0hfv-NYucqgRxDuXw4PmjSksLRMhHR5EVxfKvS4tDEmBgHSX1cxoYzXyS1A$">test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java</a>:</p>
<pre style="color:#555">> + if (args.length > 0) {
+ Policy.setPolicy(new SimplePolicy());
+ System.setSecurityManager(new SecurityManager());
+ }
</pre>
</blockquote>
</p>
<p>
<blockquote type="cite">Just curious, why split the loop into 2
and set the SecurityManager in between the two loops? Can't we
just set the policy/security manager before the loop?</blockquote>
The test infra requires various permissions including the problem
setContextClassLoader permission. I figured it was better to set
up the test infra first and then trigger the security manager.</p>
<p>New edits just pushed for review.</p>
<p>regards,<br>
Sean.<br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 25/06/2021 23:31, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite" cite="mid:yUUsn9ZWGpZUNx6NHucY5dhYMBB7bPAy6Z8zecCw_aM=.ead38451-4a61-4f49-b6e5-c9d51ae5e3db@github.com">
<pre class="moz-quote-pre" wrap="">On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey <a class="moz-txt-link-rfc2396E" href="mailto:coffeys@openjdk.org"><coffeys@openjdk.org></a> wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Sean Coffey has updated the pull request incrementally with one additional commit since the last revision:
Move TokenPoller to Runnable
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
src/java.base/share/lib/security/default.policy line 131:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.*";
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">140: permission java.security.SecurityPermission "clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission "getProperty.auth.login.defaultCallbackHandler";
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Same "avoid unnecessary changes" comment here.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 952:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">950: AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
nit: "Poller " -> "Poller-" (like before)?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 956:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">1210:
1211: this.token = token;
1212: if (cleaner == null) {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This check seems duplicate to the one in createCleaner() call.
test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Move this if-check block of code up before the for-loop?
-------------
PR: <a class="moz-txt-link-freetext" href="https://git.openjdk.java.net/jdk17/pull/117">https://git.openjdk.java.net/jdk17/pull/117</a>
</pre>
</blockquote>
</body>
</html>