Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v3]

[Sean Coffey]

> 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 with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains four additional commits since 
the last revision:

 - Edits from review
 - Merge remote-tracking branch 'origin/master' into pkcs11-perms
 - Move TokenPoller to Runnable
 - 8269034: AccessControlException for SunPKCS11 daemon threads

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/117/files
  - new: https://git.openjdk.java.net/jdk17/pull/117/files/03af6494..e961ff09

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=117=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=117=01-02

  Stats: 3102 lines in 121 files changed: 2073 ins; 670 del; 359 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/117.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/117/head:pull/117

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

[Seán Coffey]

Hi Valerie,

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.


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.


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!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


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?
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.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  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) () -> {
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) () -> {

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: 

Integrated: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.
> 
> Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

This pull request has now been integrated.

Changeset: e9b2c058
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/e9b2c058a4ed5de29b991360f78fc1c5263c9268
Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod

8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

Reviewed-by: lancea, naoto

-

PR: https://git.openjdk.java.net/jdk/pull/4615

Re: Initial feedback from Apache Ant users on recent security manager warning messages

[Alan Bateman]

On 28/06/2021 18:16, Jaikiran Pai wrote:


On a slightly related note, I was wondering why we decided to go with 
what appears to be a bit more aggressive approach to these warning 
messages as compared to what was done with the illegal reflective 
access warnings? I would have thought that the illegal reflective 
access changes would be much more involved if not the same level as 
moving away from setSecurityManager calls.
The typical SM setup will be to set it once, the Ant "same JVM" scenario 
where it sets and then resets it may be unusual.


In any case, the original implementation patch did have caching to avoid 
duplicates. It wasn't quite right and had to be pulled, it may be time 
to re-visit that to avoid too much noise for code that sets it many times.


-Alan.

Re: RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Lance Andersen]

On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.
> 
> Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

The revisions you made as part of the push to JDK 18 look fine Max.

-

Marked as reviewed by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4615

RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

More refactoring to limit the scope of `@SuppressWarnings` annotations.

Sometimes I introduce new methods. Please feel free to suggest method names you 
like to use.

Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

-

Commit messages:
 - copy all code change from jdk17

Changes: https://git.openjdk.java.net/jdk/pull/4615/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4615=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269409
  Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4615.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4615/head:pull/4615

PR: https://git.openjdk.java.net/jdk/pull/4615

[jdk17] Withdrawn: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Initial feedback from Apache Ant users on recent security manager warning messages

[Jaikiran Pai]

(resending from the correct subscribed address)

Given the recent changes around the Java SecurityManager deprecation, 
the Ant project has been asking for user feedback on how this change 
impacts them with their Ant build files/tasks. So far we have received 
two separate user reports around this. Both of them come down to the 
same issue. Before getting to that, let me provide some context around 
what Ant does with SecurityManager. I wasn't around when the intial 
design and discussions happened decades back on Ant's usage of Java 
SecurityManager, so my knowledge around this mostly based on what I see 
in the Ant project's code and its documentation.


Ant allows user defined builds to define (optional) permissions[2]. As 
you can see from that documentation, it's just a wrapper on top of what 
Java SecurityManager provides. We internally just set up the Java 
SecurityManager appropriately and invoke the relevant tasks.


Of course, "permissions" are optional and I am not sure how many of our 
users use any of them. However, there are some tasks in Ant, like the 
"java" task which by default apply certain permissions before launching 
these tasks. The internal implementation of the "java" task sets a 
custom security manager which overrides the checkPermission(...) API to 
match against the default permissions that are set for this task. All 
that code resides in the Permissions class (and its nested MySM class) 
here[3].


Many Ant tasks run within the same JVM as the one in which the Ant build 
has been triggered. Users are allowed to configure their builds to 
launch these tasks in a "forked" (separate) JVM. "java" is one such 
task. By default we do _not_ fork and instead launch the user configured 
class in the same VM as that of the Ant build. As noted earlier, when we 
do this, we first set a new security manager and then once the execution 
completes, reset the security manager back to the old one. The setting 
and resetting of the security manager happens using the 
System.setSecurityManager(...) API, from within the same (Ant project's 
internal Permissions.java) class.



With that background, let me now get to the issue that has been reported 
by more than one user. Up until these recent EA releases of Java 17, 
users who had Ant build files (some of them very large with many 
targets/tasks) had numerous build targets and many of these targets have 
the "java" task. And since this task by default doesn't fork, most of 
these build files aren't forking when this build target is executed. 
Starting these releases, these users have started to see a flood of the 
deprecation warning messages, due to Ant's (internal) calls to 
System.setSecurityManager(...).



Consider this simple Ant build file and a trivial Java program:



    
        
    

    

        

    



public class HelloWorld {

    public static void main(final String[] args) throws Exception {
        System.out.println("Hello world");
    }
}

All it does is, in its "helloworld" target, uses one single "java" task 
to launch the HelloWorld class. The output it generates (I use the term 
output loosely and don't really mean STDOUT but a combination of STDOUT 
and STDERR content that gets printed out) is as follows (this is against 
latest Java 17 build 17-ea+28-2534):



helloworld:
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by 
org.apache.tools.ant.types.Permissions 
(file:/home/me//apache-ant-1.10.9/lib/ant.jar)
WARNING: Please consider reporting this to the maintainers of 
org.apache.tools.ant.types.Permissions

WARNING: System::setSecurityManager will be removed in a future release
 [java] Hello world
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by 
org.apache.tools.ant.types.Permissions 
(file:/home/me//apache-ant-1.10.9/lib/ant.jar)
WARNING: Please consider reporting this to the maintainers of 
org.apache.tools.ant.types.Permissions

WARNING: System::setSecurityManager will be removed in a future release

There are multiple things of note here:

1. For a single line of output from the program it ended up generating 8 
additional lines of warning messages. I know this is a visual thing, but 
I wanted to highlight what I (and the users who reported this) meant by 
"flooding" of the logs. This virtually makes it almost impossible to 
find any real output from the program when someone is viewing the logs.


2. Notice that although it's the same 
org.apache.tools.ant.types.Permissions which is the caller to the 
System.setSecurityManager(...) API, it's being reported more than once. 
I want to emphasize this part too, since although you see it twice here, 
the important bit to remember is, it gets printed twice for every single 
usage of "java" task in the build file. So the more number of "java" 
tasks, the more you will see these messages pointing to 

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Mon, 28 Jun 2021 12:20:38 GMT, Daniel Fuchs  wrote:

>> This cast is only to tell the compiler which overloaded method to call, and 
>> I don't think there will be a real cast at runtime. It might look a little 
>> ugly but extracting it into a variable declaration/definition plus a new 
>> `initStatic` method seems not worth doing, IMHO.
>
> Why not simply declare a local variable in the static initializer below?
> 
> 
> private static final long CURRENT_PID;
> private static final boolean ALLOW_ATTACH_SELF;
> static {
> PrivilegedAction pa = ProcessHandle::current;
> @SuppressWarnings("removal")
> long pid = AccessController.doPrivileged(pa).pid();
> CURRENT_PID = pid;
> String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
> ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
> }

I've just pushed a commit with a different fix:

private static final long CURRENT_PID = pid();

@SuppressWarnings("removal")
private static long pid() {
PrivilegedAction pa = () -> ProcessHandle.current();
return AccessController.doPrivileged(pa).pid();
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang  wrote:

>> More refactoring to limit the scope of `@SuppressWarnings` annotations.
>> 
>> Sometimes I introduce new methods. Please feel free to suggest method names 
>> you like to use.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   one more

I'm going to move this to jdk18.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Daniel Fuchs]

On Sat, 26 Jun 2021 23:55:46 GMT, Weijun Wang  wrote:

>> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java 
>> line 53:
>> 
>>> 51: private static final long CURRENT_PID = 
>>> AccessController.doPrivileged(
>>> 52: (PrivilegedAction) 
>>> ProcessHandle::current).pid();
>>> 53: 
>> 
>> The original code separated out the declaration of the PrivilegedAction to 
>> avoid this cast. If you move the code from the original static initializer 
>> into a static method that it called from initializer then it might provide 
>> you with a cleaner way to refactor this. There are several other places in 
>> this patch that could do with similar cleanup.
>
> This cast is only to tell the compiler which overloaded method to call, and I 
> don't think there will be a real cast at runtime. It might look a little ugly 
> but extracting it into a variable declaration/definition plus a new 
> `initStatic` method seems not worth doing, IMHO.

Why not simply declare a local variable in the static initializer below?


private static final long CURRENT_PID;
private static final boolean ALLOW_ATTACH_SELF;
static {
PrivilegedAction pa = ProcessHandle::current;
@SuppressWarnings("removal")
long pid = AccessController.doPrivileged(pa).pid();
CURRENT_PID = pid;
String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v3]

[Weijun Wang]

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  one more refinement

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/152/files
  - new: https://git.openjdk.java.net/jdk17/pull/152/files/774eb9da..2e4a8ba7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=152=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=152=01-02

  Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/152.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152

PR: https://git.openjdk.java.net/jdk17/pull/152