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 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
