-
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?
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
/*[MSG "K0637", "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("K0637")); //$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.
I think throwErrorOnInit
isn't needed, the Error
can be moved around Line 1278 whenever java.security.manager
is detected.
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 liked the variable since the -Djava.security.manager=disallow
case should not throw an exception.
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 message should be prefixed with "Error: "; see SecurityManagerWarnings.java.
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.
That is from java.lang.Error
. The example output from https://openjdk.org/jeps/486 is:
java.lang.Error: A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported.
at java.lang.System.initPhase3(java.base@24/System.java:2067)
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+.
runtime/jcl/common/acccont.c
Outdated
@@ -25,6 +25,7 @@ | |||
#include "j9.h" | |||
#include "j9port.h" | |||
|
|||
#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.
This file can be removed 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.
How do I do that?
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 can be decorated w/ if(JAVA_SPEC_VERSION LESS 24)
openj9/runtime/jcl/CMakeLists.txt
Line 109 in 7576b59
${CMAKE_CURRENT_SOURCE_DIR}/common/acccont.c |
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.
Don't forget to do the equivalent for UMA builds.
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, I added that and removed the directives from acccont.c.
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 missed the UMA comment, working on that too.
@@ -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
/*[IF JAVA_SPEC_VERSION >= 24]*/ | ||
/*[MSG "K0B03", "Setting a Security Manager is not supported"]*/ | ||
throw new UnsupportedOperationException(Msg.getString("K0B03")); //$NON-NLS-1$ | ||
/*[ELSE] 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.
Space before */
, please - here and on line 1431.
@@ -44,30 +46,30 @@ | |||
@SuppressWarnings("removal") | |||
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */ | |||
public final class AccessController { | |||
/*[IF JAVA_SPEC_VERSION >= 24]*/ | |||
private static AccessControlContext ACC_NO_PERM = new AccessControlContext( |
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.
While you're here, please add final
.
static final int OBJS_INDEX_ACC = 0; | ||
static final int OBJS_INDEX_PDS = 1; | ||
static final int OBJS_ARRAY_SIZE = 3; | ||
static final int OBJS_INDEX_PERMS_OR_CACHECHECKED = 2; | ||
|
||
private static native void 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.
Since initializeInternal()
is no longer used; the implementation should be removed.
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 will be removed by the cmake/uma changes to exclude acccont.c
/*[ELSE] JAVA_SPEC_VERSION >= 24*/ | ||
return doPrivileged(action, doPrivilegedWithCombinerHelper(null)); | ||
/*[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.
Space before */
on lines 873, 875, 904, 906, etc.
/*[IF JAVA_SPEC_VERSION < 24]*/ | ||
checkPermsNPE(perms); | ||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ | ||
T result = action.run(); | ||
/*[IF JAVA_SPEC_VERSION < 24]*/ | ||
keepalive(context); | ||
keepalive(perms); | ||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ | ||
return result; |
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.
Suggest this be simplified to remove the unnecessary local for jdk24+
/*[IF JAVA_SPEC_VERSION < 24]*/
checkPermsNPE(perms);
T result = action.run();
keepalive(context);
keepalive(perms);
return result;
/*[ELSE] JAVA_SPEC_VERSION < 24 */
return action.run();
/*[ENDIF] JAVA_SPEC_VERSION < 24 */
runtime/jcl/common/acccont.c
Outdated
@@ -25,6 +25,7 @@ | |||
#include "j9.h" | |||
#include "j9port.h" | |||
|
|||
#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.
Don't forget to do the equivalent for UMA builds.
@@ -44,21 +44,24 @@ typedef enum { | |||
STATE_IMPLIED = 1 | |||
} StackWalkingStates; | |||
|
|||
#define STACK_WALK_STATE_MAGIC (void *)1 |
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.
Tabs should be used for indentation only. Please use spaces elsewhere.
This macro and the other STACK_WALK_*
macros are missing parentheses:
#define STACK_WALK_STATE_MAGIC ((void *)1)
Copyright IBM Corp. and others 2024 | ||
|
||
This program and the accompanying materials are made available under | ||
the terms of the Eclipse Public License 2.0 which accompanies this | ||
distribution and is available at https://www.eclipse.org/legal/epl-2.0/ | ||
or the Apache License, Version 2.0 which accompanies this distribution and | ||
is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
|
||
This Source Code may also be made available under the following | ||
Secondary Licenses when the conditions for such availability set | ||
forth in the Eclipse Public License, v. 2.0 are satisfied: GNU | ||
General Public License, version 2 with the GNU Classpath | ||
Exception [1] and GNU General Public License, version 2 with the | ||
OpenJDK Assembly Exception [2]. | ||
|
||
[1] https://www.gnu.org/software/classpath/license.html | ||
[2] https://openjdk.org/legal/assembly-exception.html | ||
|
||
SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 OR GPL-2.0-only WITH OpenJDK-assembly-exception-1.0 |
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 don't indent these lines.
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.
These indents seem to be standard for licensing in command line 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.
Common perhaps, but less than ideal.
<echo value=" "/> | ||
<echo value="#######################################################"/> | ||
<echo value="Running tests with command line options: $currentMode$"/> | ||
<echo value=" "/> |
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 change value=" "
to value=""
(the space isn't helpful).
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
f876cb1
to
66ebb8e
Compare
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]>
This is ready for another review. I'm working on building this with uma locally to test out those changes. |
Signed-off-by: Theresa Mammarella <[email protected]>
@@ -930,11 +957,15 @@ private static void checkPermsNPE(Permission... perms) { | |||
public static <T> T doPrivileged(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.
Please remove the space after ]
.
Related: #20563