-
Notifications
You must be signed in to change notification settings - Fork 724
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
base: master
Are you sure you want to change the base?
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
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
acbcf39
to
70eefab
Compare
I compiled these changes successfully on my machine on versions next and 21 with |
bd88c01
to
1952037
Compare
Please update the commit message for 1952037; it makes the bulk of the non-test changes, but doesn't describe them very well. |
b8478cd
to
563f9b2
Compare
Jenkins test sanity amac jdk21,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.
Line 385 in 3f7abb5
JVM_DoPrivileged(JNIEnv* env, jobject java_security_AccessController, jobject action, jboolean unknown, jboolean isExceptionAction) |
This native and its helper methods can be removed for JDK24+.
openj9/runtime/compiler/env/j9method.cpp
Lines 746 to 750 in 3f7abb5
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedAction;Ljava/security/AccessControlContext;)Ljava/lang/Object;", | |
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedExceptionAction;Ljava/security/AccessControlContext;)Ljava/lang/Object;", | |
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedAction;Ljava/security/AccessControlContext;[Ljava/security/Permission;)Ljava/lang/Object;", | |
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedExceptionAction;Ljava/security/AccessControlContext;[Ljava/security/Permission;)Ljava/lang/Object;", | |
"java/lang/NullPointerException.fillInStackTrace()Ljava/lang/Throwable;", |
We need a JIT reviewer to remove these.
These changes could be in a separated PR.
@@ -857,7 +874,11 @@ public static <T> T doPrivileged (PrivilegedExceptionAction<T> action, AccessCon | |||
*/ | |||
@CallerSensitive | |||
public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action) { | |||
/*[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.
Please be consistent with the indention pattern with earlier JPP decorations.
@@ -884,9 +905,14 @@ public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action) { | |||
public static <T> T doPrivilegedWithCombiner(PrivilegedExceptionAction<T> action) | |||
throws PrivilegedActionException | |||
{ | |||
/*[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.
Similar indention pattern comment.
@@ -962,8 +993,12 @@ public static <T> T doPrivileged(PrivilegedAction<T> action, | |||
public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action, | |||
AccessControlContext context, Permission... perms) | |||
{ | |||
/*[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.
Here as well.
@@ -1037,10 +1076,15 @@ public static <T> T doPrivilegedWithCombiner(PrivilegedExceptionAction<T> action | |||
AccessControlContext context, Permission... perms) | |||
throws PrivilegedActionException | |||
{ | |||
/*[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.
Also here
@@ -44,21 +44,25 @@ typedef enum { | |||
STATE_IMPLIED = 1 | |||
} StackWalkingStates; | |||
|
|||
#define STACK_WALK_STATE_MAGIC ((void *)1) | |||
|
|||
#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.
openj9/runtime/jcl/common/java_lang_Class.h
Lines 28 to 33 in c714a1e
typedef struct DoPrivilegedMethodArgs { | |
UDATA frameCounter; /* the frame just walked */ | |
j9object_t accControlContext; /* arg0EA[-1] - AccessControlContext */ | |
j9object_t permissions; /* arg0EA[-2] - Limited permission array */ | |
struct DoPrivilegedMethodArgs* next; /* next DoPrivilegedMethodArgs structure */ | |
} DoPrivilegedMethodArgs; |
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.
Thanks Jason. I made updates from your feedback.
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.
Tests are failing: see https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6650.
The symptoms suggest a stray extra quote, but I don't see where it originates.
#if JAVA_SPEC_VERSION < 24 | ||
#include "java_lang_Class.h" | ||
#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.
I don't think this include should be conditional. Ideally, it would be first (unconditionally), followed by includes needed for the implementation.
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 removed this exclude and fixed the test failure.
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
to a different job and exclude from 24+. Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
- Throw an error on initialization if java.security.manager attempts to add a security manager - configure setSecurityManager to always throw an UnsupportedOperationException - getSecurityManager will always return null since a security manager can't be set Signed-off-by: Theresa Mammarella <[email protected]>
... for further investigation. Signed-off-by: Theresa Mammarella <[email protected]>
Exclude helper methods related to removing the security manager including: - AccessController.initializeInternal native in acccont.c - native helper methods in java_lang_class - doPriviled*id fields from J9JavaVM struct - unused helper methods in java.security.AccessController - unused helper methods in java.security.AccessControlContext - unused helper methods in java.lang.Class java.security comment update and exclude helper methods Signed-off-by: Theresa Mammarella <[email protected]>
Related: #20563