RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]
Alexey Ivanov
aivanov at openjdk.org
Thu Oct 24 17:26:47 UTC 2024
On Thu, 24 Oct 2024 13:19:55 GMT, Sean Mullan <mullan at 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
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/Desktop.java line 713:
> 711: * {@code Info.plist}.
> 712: *
> 713: * @param printFileHandler handler
Suggestion:
* @param printFileHandler handler
*
Can we add a blank line here? It's present in the methods above.
Although there are other places below where it's missing; so not worth worrying.
src/java.desktop/share/classes/java/awt/Font.java line 1612:
> 1610: * obtained. The {@code String} value of this property is then
> 1611: * interpreted as a {@code Font} object according to the
> 1612: * specification of {@code Font.decode(String)}
Suggestion:
* specification of {@code Font.decode(String)}.
Period is missing.
src/java.desktop/share/classes/java/awt/Font.java line 1613:
> 1611: * interpreted as a {@code Font} object according to the
> 1612: * specification of {@code Font.decode(String)}
> 1613: * If the specified property is not found then null is returned instead.
Suggestion:
* If the specified property is not found, null is returned instead.
The old description didn't have ‘then’, it can be dropped. A comma to separate the conditional clause from the main one makes the sentence easier to read.
src/java.desktop/share/classes/java/awt/Font.java line 1780:
> 1778: * <p>
> 1779: * The property value should be one of the forms accepted by
> 1780: * {@code Font.decode(String)}
Suggestion:
* {@code Font.decode(String)}.
Period is missing.
src/java.desktop/share/classes/java/awt/Font.java line 1781:
> 1779: * The property value should be one of the forms accepted by
> 1780: * {@code Font.decode(String)}
> 1781: * If the specified property is not found then the {@code font}
Suggestion:
* If the specified property is not found, the {@code font}
src/java.desktop/share/classes/java/awt/MouseInfo.java line 68:
> 66: * @throws SecurityException if a security manager exists and its
> 67: * {@code checkPermission} method doesn't allow the operation
> 68: * @see GraphicsConfiguration
Is `GraphicsConfiguration` removed from the list because it's already mentioned and linked in the description above? Is it intentional?
src/java.desktop/share/classes/java/beans/Expression.java line 1:
> 1: /*
Needs copyright year update.
src/java.desktop/share/classes/java/beans/PropertyEditorManager.java line 71:
> 69: *
> 70: * @param targetType the class object of the type to be edited
> 71: * @param editorClass the class object of the editor classs
Suggestion:
* @param editorClass the class object of the editor class
Typo with an extra ‘s’. The line should remain unchanged.
src/java.desktop/share/classes/javax/swing/JTable.java line 6343:
> 6341: * <code>GraphicsEnvironment.isHeadless</code>
> 6342: * returns <code>true</code>
> 6343: * method disallows this thread from creating a print job request
Suggestion:
One more line to remove here.
src/java.desktop/share/classes/javax/swing/WindowConstants.java line 1:
> 1: /*
Needs updating the copyright year.
test/jdk/java/awt/Focus/CloseDialogActivateOwnerTest/CloseDialogActivateOwnerTest.java line 28:
> 26: @key headful
> 27: @bug 6785058
> 28: @summary Tests that an owner is activated on closing its owned dialog with the warning icon.
Does this test remain relevant without the security manager? Without the security manager, there will be no dialogs with the warning icon.
test/jdk/java/awt/regtesthelpers/process/ProcessCommunicator.java line 1:
> 1: /*
Copyright year needs updating.
test/jdk/java/beans/Introspector/7084904/Test7084904.java line 31:
> 29: * @library ..
> 30: * @run main Test7084904
> 31: * @author Sergey Malenkov
The test below `Test4683761.java` removes the `@author` tag. Should it be removed here?
test/jdk/java/beans/XMLEncoder/6777487/TestCheckedCollection.java line 1:
> 1: /*
Copyright years need updating.
test/jdk/java/beans/XMLEncoder/ReferenceToNonStaticField.java line 29:
> 27: * @test
> 28: * @bug 8060027
> 29: * @run main/othervm ReferenceToNonStaticField
Other tests in the set removed `/othervm`. Is it left intentionally?
test/jdk/java/beans/XMLEncoder/Test4631471.java line 28:
> 26: * @bug 4631471 6972468
> 27: * @summary Tests DefaultTreeModel encoding
> 28: * @run main/othervm Test4631471
Other tests in the set removed `/othervm`. Is it left intentionally?
Looks like this question applies to all tests in `XMLEncoder/` directory.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2392963469
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815168399
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815180719
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815184053
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815185648
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815187240
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815203856
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815290263
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815294333
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815320927
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815330072
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815362826
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815404801
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815414445
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815434082
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815439684
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1815440669
More information about the nio-dev
mailing list