Update java.security.manager behaviour #14329

babsingh · 2022-01-20T16:16:28Z

Consolidate security manager startup code invoked from
ClassLoader.initializeClassLoaders in System.initSecurityManager (new method).
As per the Java doc, the java.security.manager property should only be read
at startup. Example: If this property is initialized with "disallow" when the
JVM is started, then a security manager will not be set at startup and cannot
be set dynamically. Currently, this property is read in
System.setSecurityManager. If a user changes this property after startup, then
System.setSecurityManager's behaviour will change after startup. This behaviour
does not follow the Java doc and reference implementation. To fix this issue,
the java.security.manager is only read once at startup in
System.initSecurityManager.
In JDK18, System.setSecurityManager's run-time behaviour changes with
respect to the java.security.manager property. Previously, if the property
was null, the run-time behaviour was success or throws SecurityException.
In JDK18, the new behaviour is to throw UnsupportedOperationException if the
property is null. This matches the behaviour when the property is set to
"disallow".
Set java.security.manager property for impacted tests.
Explicitly handle null arguments in System.setSecurityManager.

Signed-off-by: Babneet Singh [email protected]

babsingh · 2022-01-20T16:19:27Z

Javadoc References

babsingh · 2022-01-20T16:26:47Z

In JDK18, System.setSecurityManager will throw UnsupportedOperationException if the java.security.manager property is not set (== null). So, this will break tests which incorporate System.setSecurityManager without setting the java.security.manager property. Debating whether to set the property for every impacted test or globally. @llxia Is there a way to set the java.security.manager property globally in the OpenJ9 test framework?

fyi @pshipton @tajila

pshipton · 2022-01-20T16:47:48Z

Keeping in mind that we'll need to remove all the SecurityManager tests for (likely) jdk19 and later.

llxia · 2022-01-24T16:09:26Z

Is there a way to set the java.security.manager property globally in the OpenJ9 test framework?

For Global settings, EXTRA_OPTIONS can be set in TKG https://github.com/adoptium/TKG/blob/master/openj9Settings.mk

If you want to set for each impacted test, EXTRA_OPTIONS can be set in the *.mk file in the test-related project. Similar to https://github.com/eclipse-openj9/openj9/blob/master/test/functional/Java8andUp/java8andUpSettings.mk

JasonFengJ9 · 2022-01-24T17:26:44Z

@babsingh please advise if this is ready for reviewing.

babsingh · 2022-01-24T17:42:46Z

@JasonFengJ9 This is good to review. I have kept it in the draft state because we will also need test changes. Without the test changes, OpenJ9 tests will fail with JDK18.

For the test changes, I am planning to update https://github.com/adoptium/TKG/blob/master/openj9Settings.mk as follows:

EXTRA_OPTIONS=-Djava.security.manager=allow

In JDK18, this should revert OpenJ9 behaviour to java.security.manager == null behaviour, which is seen in JDK17 and before.

llxia · 2022-01-24T19:14:54Z

FYI @renfeiw as the change may be added to TKG.

renfeiw · 2022-01-24T21:42:42Z

For the test changes, I am planning to update https://github.com/adoptium/TKG/blob/master/openj9Settings.mk as follows:
EXTRA_OPTIONS=-Djava.security.manager=allow

You probably need to use append, something like this:

EXTRA_OPTIONS +=  -Djava.security.manager=allow

jcl/src/java.base/share/classes/java/lang/System.java

babsingh · 2022-01-25T00:03:15Z

~~This PR depends on adoptium/TKG#263, which contains the test framework changes. This PR should only be merged once adoptium/TKG#263 is merged.~~ No longer applies. Refer to adoptium/TKG#263 (comment).

jcl/src/java.base/share/classes/java/lang/System.java

JasonFengJ9

The change looks good.

jcl/src/java.base/share/classes/java/lang/System.java

test/functional/cmdLineTests/proxyFieldAccess/playlist.xml

test/functional/JLM_Tests/variables.mk

1. Consolidate security manager startup code invoked from ClassLoader.initializeClassLoaders in System.initSecurityManager (new method). 2. As per the Java doc, the java.security.manager property should only be read at startup. Example: If this property is initialized with "disallow" when the JVM is started, then a security manager will not be set at startup and cannot be set dynamically. Currently, this property is read in System.setSecurityManager. If a user changes this property after startup, then System.setSecurityManager's behaviour will change after startup. This behaviour does not follow the Java doc and reference implementation. To fix this issue, the java.security.manager is only read once at startup in System.initSecurityManager. 3. In JDK18, System.setSecurityManager's run-time behaviour changes with respect to the java.security.manager property. Previously, if the property was null, the run-time behaviour was success or throws SecurityException. In JDK18, the new behaviour is to throw UnsupportedOperationException if the property is null. This matches the behaviour when the property is set to "disallow". Fixes: eclipse-openj9#14092 Fixes: eclipse-openj9#14094 Signed-off-by: Babneet Singh <[email protected]>

Tests which use System.setSecurityManager need to explicitly set the java.security.manager property in JDK18+. Impacted Java8andUp tests: - JCL_TEST_Java-Security - JCL_TEST_Java-Lang - JCL_TEST_Java-Lang-Invoke - JCL_TEST_Java-Lang_ClassLoader - JCL_TEST_Java-Internals - JCL_TEST_IBM-VM - JCL_TEST_Java-Lang-Ref Impacted JLM_Tests: - JLM_Tests_IBMinternal - TestMemoryMXBean Impacted Jsr292 tests: - jsr292Test - jsr292BootstrapTest Impacted cmdLineTests: - J9security - proxyFieldAccess Signed-off-by: Babneet Singh <[email protected]>

setSecurityManager should do nothing if the input argument is null and no security manager has been established. Before, this was being enforced implicitly. The implicit check no longer works. There is error handling code and code to print a warning message, which needs to avoided in the above "do nothing" scenario. So, an explicit null check has been added to enforce the "no nothing" scenario. Signed-off-by: Babneet Singh <[email protected]>

renfeiw

Thanks! LGTM

babsingh · 2022-02-03T21:22:04Z

@tajila can you please relaunch the PR builds?

tajila · 2022-02-03T21:35:52Z

Jenkins test sanity win jdk11

tajila · 2022-02-03T21:36:04Z

Jenkins test sanity plinux jdk17,18

tajila · 2022-02-03T21:36:20Z

Jenkins test sanity plinux jdk17,jdk18

tajila · 2022-02-04T14:21:30Z

@babsingh please make a PR for the JDK18 stream as well

babsingh · 2022-02-04T15:03:39Z

please make a PR for the JDK18 stream as well

@tajila Created #14437 for the JDK18 0.31 release branch.

The below tests have been fixed by eclipse-openj9/openj9#14329: - java/lang/System/AllowSecurityManager - java/lang/System/SecurityManagerWarnings Signed-off-by: Babneet Singh <[email protected]>

In some cases, warning messages are no longer being printed when the input argument is null. Revising the fix in eclipse-openj9#14329 to correctly throw UnsupportedOperationException (UOE) in case of a null input argument when the security manager is disallowed. When the security manager is not allowed to be set dynamically, return if the input argument is null. UnsupportedOperationException should only be thrown for a non-null input argument. Fixes: eclipse-openj9#14445 Signed-off-by: Babneet Singh <[email protected]>

babsingh added jdk11 jdk17 jdk18 jdk8 labels Jan 20, 2022

babsingh marked this pull request as draft January 20, 2022 16:17

pshipton requested a review from JasonFengJ9 January 20, 2022 16:52

pshipton added the comp:vm label Jan 20, 2022

JasonFengJ9 reviewed Jan 24, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/java/lang/System.java Show resolved Hide resolved

babsingh mentioned this pull request Jan 24, 2022

Account for java.security.manager behaviour changes in JDK18 adoptium/TKG#263

Closed

babsingh force-pushed the sm_jdk18 branch 4 times, most recently from 77c99c8 to e19d599 Compare January 24, 2022 23:59

babsingh marked this pull request as ready for review January 25, 2022 00:02

JasonFengJ9 reviewed Jan 25, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Show resolved Hide resolved

JasonFengJ9 approved these changes Jan 25, 2022

View reviewed changes

tajila reviewed Feb 1, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Show resolved Hide resolved

tajila reviewed Feb 1, 2022

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Show resolved Hide resolved

babsingh force-pushed the sm_jdk18 branch 3 times, most recently from f1ff1b9 to a411f47 Compare February 1, 2022 18:18

tajila mentioned this pull request Feb 2, 2022

Exclude correct GetCallerClassTests adoptium/aqa-tests#3311

Merged

renfeiw reviewed Feb 2, 2022

View reviewed changes

test/functional/cmdLineTests/proxyFieldAccess/playlist.xml Show resolved Hide resolved

renfeiw reviewed Feb 2, 2022

View reviewed changes

test/functional/JLM_Tests/variables.mk Outdated Show resolved Hide resolved

babsingh force-pushed the sm_jdk18 branch 4 times, most recently from ecdd3ef to cff0f94 Compare February 3, 2022 17:24

babsingh force-pushed the sm_jdk18 branch from cff0f94 to b574e3f Compare February 3, 2022 17:25

babsingh added 2 commits February 3, 2022 12:32

babsingh force-pushed the sm_jdk18 branch from b574e3f to c0e4f3a Compare February 3, 2022 17:33

renfeiw approved these changes Feb 3, 2022

View reviewed changes

tajila merged commit 9398b11 into eclipse-openj9:master Feb 4, 2022

babsingh mentioned this pull request Feb 4, 2022

[JDK18] Re-enable Security Manager Tests adoptium/aqa-tests#3318

Merged

pshipton mentioned this pull request Feb 7, 2022

OpenJDK System/SecurityManagerWarnings message missing from stderr #14445

Closed

babsingh mentioned this pull request Feb 7, 2022

Correctly throw UOE from setSecurityManager #14447

Merged

babsingh mentioned this pull request Feb 8, 2022

[0.31 release] Correctly throw UOE from setSecurityManager #14453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update java.security.manager behaviour #14329

Update java.security.manager behaviour #14329

babsingh commented Jan 20, 2022 •

edited

Loading

babsingh commented Jan 20, 2022

babsingh commented Jan 20, 2022

pshipton commented Jan 20, 2022

llxia commented Jan 24, 2022

JasonFengJ9 commented Jan 24, 2022

babsingh commented Jan 24, 2022 •

edited

Loading

llxia commented Jan 24, 2022

renfeiw commented Jan 24, 2022

babsingh commented Jan 25, 2022 •

edited

Loading

JasonFengJ9 left a comment

renfeiw left a comment

babsingh commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 4, 2022

babsingh commented Feb 4, 2022

Update java.security.manager behaviour #14329

Update java.security.manager behaviour #14329

Conversation

babsingh commented Jan 20, 2022 • edited Loading

babsingh commented Jan 20, 2022

Javadoc References

babsingh commented Jan 20, 2022

pshipton commented Jan 20, 2022

llxia commented Jan 24, 2022

JasonFengJ9 commented Jan 24, 2022

babsingh commented Jan 24, 2022 • edited Loading

llxia commented Jan 24, 2022

renfeiw commented Jan 24, 2022

babsingh commented Jan 25, 2022 • edited Loading

JasonFengJ9 left a comment

Choose a reason for hiding this comment

renfeiw left a comment

Choose a reason for hiding this comment

babsingh commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 3, 2022

tajila commented Feb 4, 2022

babsingh commented Feb 4, 2022

babsingh commented Jan 20, 2022 •

edited

Loading

babsingh commented Jan 24, 2022 •

edited

Loading

babsingh commented Jan 25, 2022 •

edited

Loading