On Thu, 24 Oct 2024 13:19:55 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 150 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge > - Update @summary to replace "if the right permission is granted" can be > replaced with "package java.lang is open to unnamed module". > - Remove println about Security Manager. > - Remove unused static variable NEW_PROXY_IN_PKG. > - Remove static variable `DEFAULT_POLICY` and unused imports. > - Remove hasSM() method and code that calls it, and remove comment about > running test manually with SM. > - clientlibs: import order > - warning-string > - java/net/httpclient/websocket/security/WSURLPermissionTest.java: > integrated review feedback in renamed WSSanityTest.java > - ... and 140 more: https://git.openjdk.org/jdk/compare/f7a61fce...cb50dfde Reviewed java/util/* and corresponding tests. Logging tests should refactor to eliminate use of doPrivileged(fn); test/jdk/java/util/PluggableLocale/PermissionTest.java line 52: > 50: import com.foo.NumberFormatProviderImpl; > 51: > 52: public class PermissionTest{ This test can be deleted entirely. What remains is just constructing each provider impl. The summary mentions a RuntimePermission and would need to be revised to a new description. test/jdk/java/util/ResourceBundle/modules/security/src/test/jdk/test/Main.java line 48: > 46: throw new RuntimeException("unexpected resource bundle"); > 47: } > 48: This main and TestPermission.java duplicate the basic resource location mechanisms. This test/Main.java an test/TestPermission can be removed entirely. test/jdk/java/util/concurrent/Executors/PrivilegedCallables.java line 28: > 26: * @bug 6552961 6558429 > 27: * @summary Test privilegedCallable, > privilegedCallableUsingCurrentClassLoader > 28: * @run main PrivilegedCallables There's nothing privileged here; the test should be renamed or deleted if it duplicates other non-privileged call tests. test/jdk/java/util/logging/FileHandlerLongLimit.java line 157: > 155: static class Configure { > 156: static void setUp(Properties propertyFile) { > 157: doPrivileged(() -> { The doPrivileged() could be factored out. And callPrivileged(). test/jdk/java/util/logging/FileHandlerPath.java line 149: > 147: static class Configure { > 148: static void setUp(Properties propertyFile) { > 149: doPrivileged(() -> { doPrivileged() could be refactored; it is no longer necessary. test/jdk/java/util/logging/FileHandlerPatternExceptions.java line 106: > 104: static class Configure { > 105: static void setUp(Properties propertyFile) { > 106: doPrivileged(() -> { doPrivileged() is no longer necessary. test/jdk/java/util/logging/TestAppletLoggerContext.java line 40: > 38: * @modules java.base/jdk.internal.access > 39: * java.logging > 40: * @run main/othervm TestAppletLoggerContext LoadingApplet Rename these? What's really being tested, there are no more Applets. test/jdk/java/util/logging/TestLogConfigurationDeadLockWithConf.java line 83: > 81: * then the test is considered a success and passes. > 82: * > 83: * Note that 4sec may not be enough to detect issues if there are > some. Where is this timeout enforced or measured; this is just a comment, not a parameter. test/micro/org/openjdk/bench/java/security/ProtectionDomainBench.java line 125: > 123: } > 124: > 125: @Benchmark Is this benchmark still useful if it is not comparing the SM vs the Non-SM case? ------------- PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2396279786 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817269899 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817285899 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817291039 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817304993 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817307433 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817310090 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817323513 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817327555 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817352471
