-
Notifications
You must be signed in to change notification settings - Fork 729
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
JDK24: Permanently Disable the Security Manager #20625
Conversation
<disables> | ||
<disable> | ||
<comment>https://github.com/eclipse-openj9/openj9/issues/20563</comment> | ||
<version>24+</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llxia is this the preferred way to disable tests from running in future versions? The security tests should run for JDK 11-23 only. I've mostly seen the block used in temporarily disabled tests so I wanted to check since this would be a permanent change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable
is for temporary excludes. In this case, we should set <version>[11, 23]</version>
. Example code: https://github.com/adoptium/TKG/blob/master/examples/jdkVersion/playlist.xml#L39
888855d
to
7ab4eda
Compare
@JasonFengJ9 Please review these changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The natives related to checkPermission()
can be ifdef
out for JDK24+ such as
openj9/runtime/jcl/common/java_lang_Class.cpp
Line 1406 in 48709bf
Java_java_security_AccessController_getAccSnapshot(JNIEnv* env, jclass jsAccessController, jint startingFrame, jboolean forDoPrivilegedWithCombiner) |
@@ -1265,6 +1265,10 @@ static void checkTmpDir() { | |||
|
|||
/*[IF JAVA_SPEC_VERSION >= 9]*/ | |||
static void initSecurityManager(ClassLoader applicationClassLoader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initSecurityManager()
can be removed at
System.initSecurityManager(applicationClassLoader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still needed since initSecurityManager
is used to detect settings of the java.security.manager
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initSecurityManager()
reads the system property java.security.manager
, and sets throwUOEFromSetSM
which can be skipped within setSecurityManager()
.
System.initSecurityManager(applicationClassLoader)
seems not needed for JDK24+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will still be needed to throw an exception on startup for illegal java.security.manager manager settings triggered by throwErrorOnInit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$ | |
if (null == javaSecurityManager) { | |
/*[IF JAVA_SPEC_VERSION >= 18]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION >= 18 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */ | |
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/*[IF JAVA_SPEC_VERSION > 11]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION > 11 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION > 11 */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 24]*/ | |
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/ | |
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$ | |
/*[ELSE] JAVA_SPEC_VERSION >= 24 */ | |
if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/* Do nothing. */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 17]*/ | |
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$ | |
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */ | |
if (javaSecurityManager.isEmpty() || "default".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
setSecurityManager(new SecurityManager()); | |
} else { | |
try { | |
Constructor<?> constructor = Class.forName(javaSecurityManager, true, applicationClassLoader).getConstructor(); | |
constructor.setAccessible(true); | |
setSecurityManager((SecurityManager)constructor.newInstance()); | |
} catch (Throwable e) { | |
/*[MSG "K0631", "JVM can't set custom SecurityManager due to {0}"]*/ | |
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$ | |
} | |
} | |
} | |
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify the benefits of this approach over the existing change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, the proposed change has 4 pairs of /*[IF JAVA_SPEC_VERSION >= 24]*/
and an unnecessary local variable throwErrorOnInit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we have is correct. This could be refactored to eliminate that local variable, but I think that should be done separately (if at all), and I can see ways forward that are even better.
jcl/src/java.base/share/classes/java/security/AccessControlContext.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/shareClassTests/DataHelperTests/playlist.xml
Outdated
Show resolved
Hide resolved
test/functional/Java8andUp/src/org/openj9/test/attachAPI/TestAttachAPI.java
Outdated
Show resolved
Hide resolved
FYI #20655 |
fce9383
to
fc9e6eb
Compare
jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, all AccessController.doPrivileged_XXX
usages can be removed for JDK24+ such as
static final boolean ENABLED = AccessController.doPrivileged(new PrivilegedAction<Boolean>() { |
Yeah, there are lots of them.
@@ -49,25 +51,25 @@ public final class AccessController { | |||
initializeInternal(); | |||
} | |||
|
|||
private static native void initializeInternal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This native can be removed for JDK24+, I expect J9JavaVM->doPrivilegedMethodID_XXX
are not needed along with
openj9/runtime/jcl/common/acccont.c
Line 28 in 3da9d2f
jboolean JNICALL Java_java_security_AccessController_initializeInternal(JNIEnv *env, jclass thisClz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have excluded these and am running a personal build to see if there are any impacted tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have excluded and opened an issue for the one failure I found #20702.
72168cc
to
c9cabc6
Compare
Do you mind if I do this in a second pull request? This change set is already getting large. |
Sounds good. |
@@ -1265,6 +1265,10 @@ static void checkTmpDir() { | |||
|
|||
/*[IF JAVA_SPEC_VERSION >= 9]*/ | |||
static void initSecurityManager(ClassLoader applicationClassLoader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$ | |
if (null == javaSecurityManager) { | |
/*[IF JAVA_SPEC_VERSION >= 18]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION >= 18 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */ | |
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/*[IF JAVA_SPEC_VERSION > 11]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION > 11 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION > 11 */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 24]*/ | |
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/ | |
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$ | |
/*[ELSE] JAVA_SPEC_VERSION >= 24 */ | |
if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/* Do nothing. */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 17]*/ | |
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$ | |
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */ | |
if (javaSecurityManager.isEmpty() || "default".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
setSecurityManager(new SecurityManager()); | |
} else { | |
try { | |
Constructor<?> constructor = Class.forName(javaSecurityManager, true, applicationClassLoader).getConstructor(); | |
constructor.setAccessible(true); | |
setSecurityManager((SecurityManager)constructor.newInstance()); | |
} catch (Throwable e) { | |
/*[MSG "K0631", "JVM can't set custom SecurityManager due to {0}"]*/ | |
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$ | |
} | |
} | |
} | |
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ | |
} | |
} |
@@ -637,6 +651,7 @@ private static int getNewAuthorizedState(AccessControlContext acc, ProtectionDom | |||
} | |||
return newAuthorizedState; | |||
} | |||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to L722 to include toArrayOfProtectionDomains()
.
checkPermsNPE(perms); | ||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keepalive()
can be removed for JDK24+.
@@ -55,10 +55,12 @@ typedef enum { | |||
#define STACK_WALK_STATE_LIMITED_DOPRIVILEGED (void *)2 | |||
#define STACK_WALK_STATE_FULL_DOPRIVILEGED (void *)3 | |||
|
|||
#if JAVA_SPEC_VERSION < 24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjsArraySizeNindex
, STACK_WALK_STATE_LIMITED_DOPRIVILEGED
and STACK_WALK_STATE_FULL_DOPRIVILEGED
can be removed for JDK24+.
ac1f215
to
4f82040
Compare
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
25b945a
to
f876cb1
Compare
I think OpenJ9 |
Agreed. |
If that were true, it wouldn't execute the next line: Assert.fail("java.lang.System.security shoud NOT be accessible via reflection"); but we do see that failure message in the log. |
I just chatted with Keith and we were in agreement on this approach. For now I will disable the test and do this another time since it will add quite a bit to this pull request. I've started a list of follow up changes #20563 (comment) |
... from Java8andUp test_getDeclaredFieldLjava_lang_String Signed-off-by: Theresa Mammarella <[email protected]>
The suggestion is fine with me. We could also modify the test to look for System.initSecurityManager. I'd lean towards modifying the test, but don't have a strong opinion.
It makes the code more cryptic, although a comment would help with that. |
I excluded the failing test in test_getDeclaredFieldLjava_lang_String. java/lang/System/SecurityManagerWarnings.java.SecurityManagerWarnings still needs to be resolved. |
8926087
to
e998852
Compare
67adc10
to
cc0f67c
Compare
Jenkins test sanity amac jdk21,jdknext |
java/lang/System/SecurityManagerWarnings.java is still failing. I don't think the other failures are related. The test is looking for "at java.lang.System.initPhase3" and j9 prints "at java/lang/System.initPhase3":
I will update the test to fix this, and while I'm at it I think we should keep the |
The failing job links before my latest push are: |
Signed-off-by: Theresa Mammarella <[email protected]>
Jenkins test sanity amac jdknext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the test failure is related to this change.
Related: #20563