Re: RFR: 8345799: Update copyright year to 2024 for core-libs in files where it was missed [v2]
On Mon, 9 Dec 2024 13:03:06 GMT, Magnus Ihse Bursie wrote: >> Some files have been modified in 2024, but the copyright year has not been >> properly updated. This should be fixed. >> >> I have located these modified files using: >> >> git log --since="Jan 1" --name-only --pretty=format: | sort -u > file.list >> >> and then run a script to update the copyright year to 2024 on these files. >> >> I have made a manual sampling of files in the list to verify that they have >> indeed been modified in 2024. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add more core-libs files I also meant to note that there are updates to binaries, src/java.base/share/classes/jdk/internal/icu/impl/data/icudt76b/... which maybe isn't intentional, or I just don't understand? - PR Comment: https://git.openjdk.org/jdk/pull/22640#issuecomment-2528357715
Re: RFR: 8345799: Update copyright year to 2024 for core-libs in files where it was missed [v2]
On Mon, 9 Dec 2024 13:03:06 GMT, Magnus Ihse Bursie wrote: >> Some files have been modified in 2024, but the copyright year has not been >> properly updated. This should be fixed. >> >> I have located these modified files using: >> >> git log --since="Jan 1" --name-only --pretty=format: | sort -u > file.list >> >> and then run a script to update the copyright year to 2024 on these files. >> >> I have made a manual sampling of files in the list to verify that they have >> indeed been modified in 2024. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add more core-libs files Looks good. They all seem to follow the pattern, I looked at some and yes they have changes in 2024 but no year update. One of them was mine I noticed! - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22640#pullrequestreview-243450
Re: RFR: 8345565: Remove remaining SecurityManager motivated APIs from sun.reflect.util
On Thu, 5 Dec 2024 10:20:50 GMT, Alan Bateman wrote: > We hollowed out ReflectUtil as one of the early steps when removing the code > for running in the SecurityManager execution mode. Most of the usages have > now been removed so the empty (and unused) methods can be removed. FieldUtils > and ConstructorUtils can be removed too. > > ObjectInputStream/ObjectOutputStream has a left over package access check for > the subclassing case that can be removed. > > sun.reflect.generics.reflectiveObjects.TypeVariableImpl.getGenericDeclaration > has a left over package access check that can be removed. I've changed the > "should not happen" case to be an assert for now but it's in the wrong place. > If we have a JDK bug in this area then it should be caught at construction > time, not by the accessor method. This PR is focused on removing the use of > ReflectUtil so don't want to do any more here. > > The changes for java.management missed a usage ConstructorUtil.getConstructor > in MBeanInstantiator.findConstructor. This is replaced, to allow > ConstructorUtils be removed. > > Testing: tier1-5 The java.management change/removal looks good to me. - PR Comment: https://git.openjdk.org/jdk/pull/22572#issuecomment-2520051019
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v9]
On Tue, 3 Dec 2024 09:03:20 GMT, Jaikiran Pai wrote: > Although trivial, there are some changes to files from the serviceability > area. So it would be good if someone from that area could review this too. Yes, looks good. I will update https://github.com/openjdk/jdk/pull/22478 to avoid the clash! - PR Comment: https://git.openjdk.org/jdk/pull/22478#issuecomment-2516659181
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v9]
On Tue, 3 Dec 2024 06:12:13 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which removes usages of >> SecurityManager related APIs and some leftover related to SecurityManager >> changes? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these >> changes are trivial. The >> `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to >> expose utility methods for dealing with SecurityManager permissions and it >> was called from a few places. That class is no longer needed with the clean >> up done in this PR. >> >> No new tests have been introduced and tier testing is currently in progress. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - merge latest from master branch > - remove permission text from InnocuousThread > - merge latest from master branch > - remove changes to > src/java.base/unix/classes/sun/security/provider/NativePRNG.java > - remove unused import > - replace remaining Paths.get() with Path.of() in the updated files > - Update > src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java > >Co-authored-by: Severin Gehwolf > - Update > src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java > >Co-authored-by: Severin Gehwolf > - Path.of() instead of Paths.get() in CgroupV2Subsystem.java > - remove unnecessary space > - ... and 5 more: https://git.openjdk.org/jdk/compare/a3b58ee5...d53fe8ad Marked as reviewed by kevinw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22478#pullrequestreview-2477811501
Re: RFR: 8337199: Add jcmd Thread.vthread_scheduler and Thread.vthread_pollers diagnostic commands [v2]
On Thu, 28 Nov 2024 15:52:01 GMT, Alan Bateman wrote:
>> test/hotspot/jtreg/serviceability/dcmd/thread/VThreadCommandsTest.java line
>> 96:
>>
>>> 94: .shouldContain("Read I/O pollers:")
>>> 95: .shouldContain("Write I/O pollers:")
>>> 96: .shouldMatch("^\\[0\\] sun.nio.ch..+ \\[registered =
>>> [\\d]+, owner = .+\\]$");
>>
>> Just a nit but are there three dots here in sun.nio.ch. that should
>> literally match, so need the \\
>> and then the other two other dots with a plus sign, to match one or more
>> characters.
>
> Well spotted, these should be escaped although it not doing so won't impact
> the match here.
Yes -it was almost the definitive bikeshedding comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22414#discussion_r1862430118
Re: RFR: 8337199: Add jcmd Thread.vthread_scheduler and Thread.vthread_pollers diagnostic commands [v2]
On Thu, 28 Nov 2024 15:54:54 GMT, Alan Bateman wrote: >> Adds `jcmd Thread.vthread_scheduler` to print the virtual thread >> scheduler and `jcmd Thread.vthread_pollers` to print the I/O pollers >> that support virtual threads doing blocking network I/O operations. >> >> This is a subset of the diagnostics that we've had in the loom repo for a >> long time. @larry-cable proposed a PR recently >> ([pull/22121](https://github.com/openjdk/jdk/pull/22121)) to bring a version >> of same into main line but it was based on an older proposal. This new PR >> supplants that effort. >> >> The jtreg failure handler is updated to execute Thread.vthread_scheduler, >> useful capture when a test fails or times out. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Improve regex for matching poller String representation Marked as reviewed by kevinw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22414#pullrequestreview-2468508549
Re: RFR: 8337199: Add jcmd Thread.vthread_scheduler and Thread.vthread_pollers diagnostic commands
On Wed, 27 Nov 2024 15:59:17 GMT, Alan Bateman wrote:
> Adds `jcmd Thread.vthread_scheduler` to print the virtual thread
> scheduler and `jcmd Thread.vthread_pollers` to print the I/O pollers
> that support virtual threads doing blocking network I/O operations.
>
> This is a subset of the diagnostics that we've had in the loom repo for a
> long time. @larry-cable proposed a PR recently
> ([pull/22121](https://github.com/openjdk/jdk/pull/22121)) to bring a version
> of same into main line but it was based on an older proposal. This new PR
> supplants that effort.
>
> The jtreg failure handler is updated to execute Thread.vthread_scheduler,
> useful capture when a test fails or times out.
test/hotspot/jtreg/serviceability/dcmd/thread/VThreadCommandsTest.java line 96:
> 94: .shouldContain("Read I/O pollers:")
> 95: .shouldContain("Write I/O pollers:")
> 96: .shouldMatch("^\\[0\\] sun.nio.ch..+ \\[registered =
> [\\d]+, owner = .+\\]$");
Just a nit but are there three dots here in sun.nio.ch. that should literally
match, so need the \\
and then the other two other dots with a plus sign, to match one or more
characters.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22414#discussion_r1862284094
Re: RFR: 8344149: Remove usage of Security Manager from java.rmi
On Fri, 15 Nov 2024 01:12:34 GMT, Stuart Marks wrote: > First cut at removal of Security Manager stuff from RMI. > > This covers just about every SM-related case in RMI, except for a bit of > package checking in MarshalInputStream. This will be handled separately. It's > covered by [JDK-8344329](https://bugs.openjdk.org/browse/JDK-8344329). > > Further simplifications could be done in RuntimeUtil and NewThreadAction. > However, those changes started to become somewhat more intrusive than I'd > like for this PR, which is focused on removing security-related stuff. Looks good to me. 8-) - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22129#pullrequestreview-2443275232
Re: RFR: 8344056: Use markdown format for man pages
On Wed, 13 Nov 2024 17:05:25 GMT, Magnus Ihse Bursie wrote: > Currently, the man pages are stored as troff (a text format) in the open > repo, and a content-wise identical copy is stored as markdown (another text > format) in the closed repo. > > Since markdown is preferred to troff in terms of editing, we make changes to > the man pages in markdown and then convert it to troff. > > This closed-markdown to open-troff processing needs to be done manually by an > Oracle engineer. This is done regularly at the start and end of a new release > cycle, adding to the burden of creating a new release. It is also done (if > any of the reviewers knows about the process) whenever an Oracle engineer > updates a man page. If a community contributor changes the behavior of a > tool, an Oracle engineer needs to change the documentation for them, since > they cannot do it themselves. I think this means the one-true-master copy of a man page is now the public .md file. All contributors can now change and improve man pages, and would be expected to make necessary man page updates when making other changes. (Which is great, I just didn't see it being explicitly said.) - PR Comment: https://git.openjdk.org/jdk/pull/22081#issuecomment-2474701337
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v11]
On Tue, 12 Nov 2024 14:44:55 GMT, Sean Mullan 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 230 commits: > > - Merge > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Move remaining JEP 486 failing tests into correct groups. > - Move JEP 486 failing tests into hotspot_runtime group. > - ... and 220 more: https://git.openjdk.org/jdk/compare/8a2a75e5...7c996a5e Marked as reviewed by kevinw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2429813961
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v10]
On Tue, 12 Nov 2024 13:01:33 GMT, Sean Mullan 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 229 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Move remaining JEP 486 failing tests into correct groups. > - Move JEP 486 failing tests into hotspot_runtime group. > - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing > - ... and 219 more: https://git.openjdk.org/jdk/compare/2ec35808...b7b95a40 Marked as reviewed by kevinw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2429727015
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan 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 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 Reviewing for management, src and test changes look good. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2382585178
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan 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 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 Thanks for updating the wording on the JMX/management classes, looks good. src/java.management/share/classes/javax/management/remote/JMXConnectorFactory.java No longer has any changes other than (C), so that should be reverted also. 8-) - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2423214468
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 20:28:28 GMT, Dhamoder Nalla wrote: >> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code >> across the OpenJDK repository to retrieve the temporary directory path, as >> GetTempPath2 provides enhanced security. While GetTempPath may still >> function without errors, using GetTempPath2 reduces the risk of potential >> exploits for users. >> >> >> The code to dynamically load GetTempPath2 is duplicated due to the following >> reasons. I would appreciate any suggestions to remove the duplication where >> possible: >> >> 1. The changes span across four different folders—java.base, jdk.package, >> jdk.attach, and hotspot—with no shared code between them. >> 2. Some parts of the code use version A, while others use version W (ANSI >> vs. Unicode). >> 3. Some parts of the code are written in C others in C++. > > Dhamoder Nalla has updated the pull request incrementally with one additional > commit since the last revision: > > fix missing code Right, so within a particular environment you can try and have matching JDK versions, but in the world as a whole we do care about different versions being able to attach to each other. (If JVMs have different temp dirs, they won't find each other and attach.) Changing the temp dir for SYSTEM processes on Windows will mean they can't be found by a SYSTEM process from a previous JDK version (assuming that SYSTEM processes already have a different temp dir to regular user processes). That may be a vanishingly small amount of processes out in the world affected, so the incompatibility risk may be small. But also if there are no processes that benefit, is it worth the complexity of the change. Maybe we can think of some examples of processes affected. How would you go about running a Java app in this way, and can we say anything about how common it is? - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2376597466
Re: RFR: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
On Mon, 16 Sep 2024 09:21:22 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which replaces the usage > of `-noclassgc` with `-Xnoclassgc` option when launching the test? > > As noted in https://bugs.openjdk.org/browse/JDK-8340176, the `-noclassgc` is > an undocumented option of the `java` launcher, which it converts to > `-Xnoclassgc` before passing it to the JVM. Instead of that option, the > documented `-Xnoclassgc` should be used. > > No new test has been introduced and the modified test continues to pass after > this change. Looks good, thanks 8-) - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21012#pullrequestreview-2306160592
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 20:28:28 GMT, Dhamoder Nalla wrote: >> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code >> across the OpenJDK repository to retrieve the temporary directory path, as >> GetTempPath2 provides enhanced security. While GetTempPath may still >> function without errors, using GetTempPath2 reduces the risk of potential >> exploits for users. >> >> >> The code to dynamically load GetTempPath2 is duplicated due to the following >> reasons. I would appreciate any suggestions to remove the duplication where >> possible: >> >> 1. The changes span across four different folders—java.base, jdk.package, >> jdk.attach, and hotspot—with no shared code between them. >> 2. Some parts of the code use version A, while others use version W (ANSI >> vs. Unicode). >> 3. Some parts of the code are written in C others in C++. > > Dhamoder Nalla has updated the pull request incrementally with one additional > commit since the last revision: > > fix missing code OK thanks, so the change only affects SYSTEM accounts, and such accounts already see a different temp path to non-SYSTEM accounts. Newer and older Java versions run by a SYSTEM account will have different temp paths, therefore the hsperfdata_username will be in a different place. (I was being picky about attach, but it's a universal thing which is expected to work across different Java versions.) Do we have any apps commonly run as a Windows SYSTEM account which expect to attach to other Java apps run by a different Java version? You're suggesting that will have no impact and agreed it would seem really unusual. 8-) - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2343034670
Re: RFR: 8338890: Add monitoring/management interface for the virtual thread scheduler [v4]
On Mon, 9 Sep 2024 06:28:45 GMT, Alan Bateman wrote: >> This PR proposes to add a JDK-specific monitoring and management interface >> for the virtual thread scheduler. The interface is named >> [VirtualThreadSchedulerMXBean](https://download.java.net/java/early_access/loom/docs/api/jdk.management/jdk/management/VirtualThreadSchedulerMXBean.html) >> and allows JMX based tooling monitor/manage the scheduler's target >> parallelism and monitor thread counts. >> >> The JDK 5/6 era JDK-specific management interfaces are in >> jdk.management/com.sun.management. The proposal here is to start afresh in >> the jdk.management package, thus establishing a "new home" for JDK-specific >> management interfaces going forward. >> >> jdk.test.lib.thread.VThreadRunner is test infrastructure used by several >> tests to set or ensure some level of parallelism. This is changed from using >> deep reflection to using the MXBean to access the target parallelism, which >> requires changes to the `@modules` tag tests in a number of tests (some >> tests no longer need to open java.base/java.lang). >> >> (The changes proposed here have been in the loom repo for some time. A >> related command for the jcmd tool is also in that repo and will be proposed >> separately). > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Remove unused code, expand testing > - Merge > - Sync up from loom repo > - Merge > - Sync up from loom repo > - Merge > - Sync up from loom repo > - Initial commit Updates look good to me. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20816#pullrequestreview-2289417860
Re: RFR: 8338890: Add monitoring/management interface for the virtual thread scheduler [v3]
On Thu, 5 Sep 2024 17:54:46 GMT, Alan Bateman wrote:
>> This PR proposes to add a JDK-specific monitoring and management interface
>> for the virtual thread scheduler. The interface is named
>> [VirtualThreadSchedulerMXBean](https://download.java.net/java/early_access/loom/docs/api/jdk.management/jdk/management/VirtualThreadSchedulerMXBean.html)
>> and allows JMX based tooling monitor/manage the scheduler's target
>> parallelism and monitor thread counts.
>>
>> The JDK 5/6 era JDK-specific management interfaces are in
>> jdk.management/com.sun.management. The proposal here is to start afresh in
>> the jdk.management package, thus establishing a "new home" for JDK-specific
>> management interfaces going forward.
>>
>> jdk.test.lib.thread.VThreadRunner is test infrastructure used by several
>> tests to set or ensure some level of parallelism. This is changed from using
>> deep reflection to using the MXBean to access the target parallelism, which
>> requires changes to the `@modules` tag tests in a number of tests (some
>> tests no longer need to open java.base/java.lang).
>>
>> (The changes proposed here have been in the loom repo for some time. A
>> related command for the jcmd tool is also in that repo and will be proposed
>> separately).
>
> Alan Bateman has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Sync up from loom repo
> - Merge
> - Sync up from loom repo
> - Merge
> - Sync up from loom repo
> - Initial commit
This all looks good to me, good to have the info and control available via JMX.
jdk.management looks like the right home.
The ObjectName String "jdk.management:type=VirtualThreadScheduler" could have a
public static final definition somewhere in future, but there is no obvious
home, e.g. java.lang.management.ManagementFactory is in the wrong package.
Something for the future, not for this change, as I others that don't have such
a symbol.
src/jdk.management/share/classes/jdk/management/VirtualThreadSchedulerMXBean.java
line 34:
> 32:
> 33: /**
> 34: * Management interface for the JDK's {@linkplain Thread##virtual-threads
> virtual thread}
is this "Management interface for **a**" virtual thread scheduler, as in future
it may be the interface generically for virtual thread schedulers?
-
Marked as reviewed by kevinw (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20816#pullrequestreview-2285923893
PR Review Comment: https://git.openjdk.org/jdk/pull/20816#discussion_r1746924898
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 20:28:28 GMT, Dhamoder Nalla wrote: >> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code >> across the OpenJDK repository to retrieve the temporary directory path, as >> GetTempPath2 provides enhanced security. While GetTempPath may still >> function without errors, using GetTempPath2 reduces the risk of potential >> exploits for users. >> >> >> The code to dynamically load GetTempPath2 is duplicated due to the following >> reasons. I would appreciate any suggestions to remove the duplication where >> possible: >> >> 1. The changes span across four different folders—java.base, jdk.package, >> jdk.attach, and hotspot—with no shared code between them. >> 2. Some parts of the code use version A, while others use version W (ANSI >> vs. Unicode). >> 3. Some parts of the code are written in C others in C++. > > Dhamoder Nalla has updated the pull request incrementally with one additional > commit since the last revision: > > fix missing code Hi, >From the linked doc: "When calling this function from a process running as SYSTEM it will return the path C:\Windows\SystemTemp, which is inaccessible to non-SYSTEM processes. For non-SYSTEM processes, GetTempPath2 will behave the same as GetTempPath." If SYSTEM and regular user processes have a different temp path, and we change temp as it's known to the attach api, they will have a different hsperfdata_username locations. Then does the attach api only find processes that are of the same category (SYSTEM vs non-SYSTEM)? e.g. "jps" for a user does not show SYSTEM processes? SYSTEM is not the "Administrator" user, so it's not going to be a very common problem, but if a Java process run as a SYSTEM then it could be an issue (is that possible?). And if Java can't be run as a SYSTEM task, then the doc states GetTempPath2 will behave the same as GetTempPath. - PR Review: https://git.openjdk.org/jdk/pull/20600#pullrequestreview-2250236104
Re: RFR: 8336254: Virtual thread implementation + test updates [v2]
On Fri, 19 Jul 2024 16:59:54 GMT, Alan Bateman wrote: >> Bringover some of the changes accumulated in the loom repo to the main line, >> most of these changes are test updates and have been baking in the loom repo >> for several months. The motive is partly to reduce the large set of changes >> that have accumulated in the loom repo, and partly to improve robustness and >> test coverage in the main line. The changes don't include any of the larger >> changes in the loom repo that are part of future JEPs. >> >> Implementation: >> - Robustness improvements to not throw OOME when unparking a virtual thread. >> - Robustness improvements to reduce class loading when a virtual thread >> parks or parks when pinned (jdk.internal.misc.VirtualThreads is removed, >> jdk.internal.event.VirtualThreadPinnedEvent is eagerly loaded) >> - VirtualThread changes to reduce contention on timer queues when doing >> timed-park >> >> Tests: >> - New tests for monitor enter/exit/wait/notify (this is a subset of the >> tests in the loom repo, we can't move many tests because they depend on on >> the changes to the object monitor implementation) >> - New test infrastructure to allow tests use a custom scheduler. This >> updates many tests to make use of this infrastructure, the "local" >> ThreadBuidlers is removed. >> - More test scenarios added to ThreadAPI and JVMTI GetThreadStateTest.java >> - New test for ThreadMXBean.getLockedMonitor with synchronized native methods >> - Reimplement of JVMTI VThreadEvent test to improve reliability >> - Rename some tests to get consistent naming >> - Diagnostic output in several stress tests to help trace progress in the >> event of a timeout >> >> Testing: tier1-6 > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - Fix typo in comment, missing copyright update, test nits > - Merge > - Drop JLA updates for this update > - Merge > - Merge > - Update copyright headers > - Initial commit The JMX/ThreadMXBean test updates look good to me. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20143#pullrequestreview-2193566771
RFR: 8335684: ThreadCpuTime.java should pause like ThreadCpuTimeArray.java
There are two similarly names tests. Recently: JDK-8335124: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed with CPU time out of expected range ...made a simple change to try and avoid noisy test failures. The same fix should be applied here to ThreadCpuTime.java. Also removing an old comment about a Solaris issue. - Commit messages: - 8335684: ThreadCpuTime.java should pause like ThreadCpuTimeArray.java Changes: https://git.openjdk.org/jdk/pull/20025/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20025&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335684 Stats: 12 lines in 1 file changed: 2 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20025/head:pull/20025 PR: https://git.openjdk.org/jdk/pull/20025
Re: RFR: 8334287: Man page update for jstatd deprecation
On Fri, 21 Jun 2024 14:05:51 GMT, Kevin Walls wrote: > Man page update for JDK-8327793 which marked jstatd as deprecated for removal > in JDK 24. Thanks Alan and David, moving to a single line: JSTATD(1) JDK CommandsJSTATD(1) NAME jstatd - monitor the creation and termination of instrumented Java HotSpot VMs SYNOPSIS WARNING: This command is experimental, unsupported, and deprecated. It will be removed in a future release. jstatd [options] - PR Comment: https://git.openjdk.org/jdk/pull/19829#issuecomment-2188272502
Re: RFR: 8334287: Man page update for jstatd deprecation [v2]
> Man page update for JDK-8327793 which marked jstatd as deprecated for removal > in JDK 24. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: text update - Changes: - all: https://git.openjdk.org/jdk/pull/19829/files - new: https://git.openjdk.org/jdk/pull/19829/files/55f6dbea..6c41666a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19829&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19829&range=00-01 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19829.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19829/head:pull/19829 PR: https://git.openjdk.org/jdk/pull/19829
Re: RFR: 8334287: Man page update for jstatd deprecation
On Fri, 21 Jun 2024 14:05:51 GMT, Kevin Walls wrote: > Man page update for JDK-8327793 which marked jstatd as deprecated for removal > in JDK 24. The updated man page looks like: JSTATD(1) JDK CommandsJSTATD(1) NAME jstatd - monitor the creation and termination of instrumented Java HotSpot VMs SYNOPSIS WARNING: This command is deprecated and will be removed in a future release. Note: This command is experimental and unsupported. jstatd [options] ...etc... - PR Comment: https://git.openjdk.org/jdk/pull/19829#issuecomment-2182976031
RFR: 8334287: Man page update for jstatd deprecation
Man page update for JDK-8327793 which marked jstatd as deprecated for removal in JDK 24. - Commit messages: - 8334287: Man page update for jstatd deprecation Changes: https://git.openjdk.org/jdk/pull/19829/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19829&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334287 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19829.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19829/head:pull/19829 PR: https://git.openjdk.org/jdk/pull/19829
Integrated: 8327793: Deprecate jstatd for removal
On Tue, 11 Jun 2024 13:09:06 GMT, Kevin Walls wrote: > jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. This pull request has now been integrated. Changeset: 9f8de221 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/9f8de221d7f0186718411ab3f5217e3883237e84 Stats: 6 lines in 3 files changed: 3 ins; 0 del; 3 mod 8327793: Deprecate jstatd for removal Reviewed-by: alanb, cjplummer - PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8327793: Deprecate jstatd for removal [v7]
On Fri, 21 Jun 2024 13:13:38 GMT, Kevin Walls wrote: >> jstatd is an RMI server application which monitors HotSpot VMs, and provides >> an interface to the monitoring tool jstat, for use across a remote RMI >> connection. >> >> RMI is not how modern applications communicate. It is an old transport with >> long term security concerns, and configuration difficulties with firewalls. >> >> The jstatd tool should be removed. Deprecating and removing jstatd will not >> affect usage of jstat for monitoring local VMs using the Attach API. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > caps /INTEGRATE - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2182738142
Re: RFR: 8327793: Deprecate jstatd for removal [v7]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: caps - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/1a240155..db53d078 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
On Fri, 21 Jun 2024 12:28:20 GMT, Alan Bateman wrote: >> For the Security Manager, the warning was worded a little differently: >> >> "WARNING: The Security Manager is deprecated and will be removed in a future >> release" >> >> I think that wording is more clear. The current wording could be confusing >> because JDK 24 is when jstatd is deprecated for removal, and not a future >> release. The wording above is more definitive for those that may not >> understand what "deprecated for removal" means. Thus, I suggest: >> >> "WARNING: jstatd is deprecated and will be removed in a future release" > > @kevinjwalls Are you planning to move to "WARNING" as has been suggested in > the previous comments. No, I was just pausing while the pre-submit checks ran again. I did investigate, the JDK seems fairly evenly split on WARNING vs Warning. This message should not be around for many releases. - PR Review Comment: https://git.openjdk.org/jdk/pull/19658#discussion_r1648922812
Re: RFR: 8327793: Deprecate jstatd for removal [v6]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: wording - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/3c7e86a1..1a240155 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8327793: Deprecate jstatd for removal [v5]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8327793_jstatd_deprecate - Remove annotations - Replace (C) - Remove annotations - No annotations for src/jdk.jstatd/share/classes/sun/jvmstat/monitor/remote - Remove tmp file - Annotations - Annotations - Merge remote-tracking branch 'upstream/master' into 8327793_jstatd_deprecate - Basic deprecation of module, jstatd prints warning. - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/ed386505..3c7e86a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=03-04 Stats: 27075 lines in 733 files changed: 18445 ins; 5705 del; 2925 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
[jdk23] Integrated: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Thu, 20 Jun 2024 15:24:35 GMT, Kevin Walls wrote: > 844: JMX attaching of Subject does not work when security manager not > allowed This pull request has now been integrated. Changeset: 23f2c97f Author: Kevin Walls URL: https://git.openjdk.org/jdk/commit/23f2c97f4ce42316fd51290175b01b41bcc24f6b Stats: 162 lines in 17 files changed: 113 ins; 3 del; 46 mod 844: JMX attaching of Subject does not work when security manager not allowed Reviewed-by: dfuchs Backport-of: bcf4bb4882e06d8c52f6eb4e9c4e027ba0622c5f - PR: https://git.openjdk.org/jdk/pull/19810
Re: [jdk23] RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Thu, 20 Jun 2024 15:24:35 GMT, Kevin Walls wrote: > 844: JMX attaching of Subject does not work when security manager not > allowed Thanks Daniel. Testing automated and manual looks good in 24 and in this 23 backport, so I'll get this integrated. - PR Comment: https://git.openjdk.org/jdk/pull/19810#issuecomment-2182392102
[jdk23] RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
844: JMX attaching of Subject does not work when security manager not allowed - Commit messages: - Backport bcf4bb4882e06d8c52f6eb4e9c4e027ba0622c5f Changes: https://git.openjdk.org/jdk/pull/19810/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19810&range=00 Issue: https://bugs.openjdk.org/browse/JDK-844 Stats: 162 lines in 17 files changed: 113 ins; 3 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/19810.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19810/head:pull/19810 PR: https://git.openjdk.org/jdk/pull/19810
Integrated: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Mon, 10 Jun 2024 11:28:28 GMT, Kevin Walls wrote: > JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. This pull request has now been integrated. Changeset: bcf4bb48 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/bcf4bb4882e06d8c52f6eb4e9c4e027ba0622c5f Stats: 162 lines in 17 files changed: 113 ins; 3 del; 46 mod 844: JMX attaching of Subject does not work when security manager not allowed Reviewed-by: weijun, dfuchs - PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]
On Tue, 18 Jun 2024 12:17:46 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Additional test runs with SM enabled Integrating, thanks for all the comments. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2179099175
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]
On Wed, 19 Jun 2024 11:21:45 GMT, Daniel Fuchs wrote: > The code changes look good to me (if a bit verbose) and the test changes look > reasonable. It could be beneficial to add some more tests in the future > involving monitoring and getting the subject from within a monitored MBean. Yes, agreed. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2178461246
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Tue, 18 Jun 2024 11:33:51 GMT, Daniel Fuchs wrote: >> Agree with Kevin. To minimize risk, especially since this is to fix a >> significant regression and we are in RDP1, we are really trying to preserve >> the existing code as much as possible, even though it could be improved. > > It is also more error prone (which is why I suggested using a single well > tested utility method to implement the temporary hackery rather than > spreading it at different places in different forms). But I'm OK with the > code in this form. Thanks Daniel! - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644374757
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Additional test runs with SM enabled - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/384a3a19..697fc0d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=18 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=17-18 Stats: 7 lines in 3 files changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > braces Agreed that this is niche and will be short-lived, but I have added some SM-enabled runs with all permission policy for ThreadPoolAccTest.java and StartStopTest.java. These look trivial to add, and will be trivial to remove... Looking forward to more cleanup when we remove the SM-enabled paths. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175956243
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> braces
Yes, maybe we are light on testing with an SM actually enabled.
AuthorizationTest is the key test here, and tests authenticated connections
with user/role names. That is passing with no SM, SM allowed, and SM enabled
with policy.
I am testing ThreadPoolAccTest.java with SM enabled with an allPermission
policy, as well as just SM allowed or not allowed. This is a good test as it
exercises the Monitor class. This still works, will add it.
Also, manual testing looks good to me:
In problem builds of jdk 23, attaching with JMX using authentication results in:
org.openjdk.kjdb.MyDebuggerException: getSubject is supported only if a
security manager is allowed
With this change, JMX attach using authentication works. A monitor role can
correctly get refusals like:
Caused by: java.lang.SecurityException: Access denied! Invalid access level for
requested MBeanServer operation.
at
com.sun.jmx.remote.security.MBeanServerFileAccessController.checkAccess(MBeanServerFileAccessController.java:348)
at
com.sun.jmx.remote.security.MBeanServerFileAccessController.checkWrite(MBeanServerFileAccessController.java:240)
at
com.sun.jmx.remote.security.MBeanServerAccessController.invoke(MBeanServerAccessController.java:469)
at
javax.management.remote.rmi.RMIConnectionImpl.doOperation(RMIConnectionImpl.java:1520)
...and a control role is accepted (that's JMX simple security at work).
Running the target with a SecurityManager, and attaching, I see e.g.:
org.openjdk.kjdb.MyDebuggerException: access denied
("javax.management.MBeanPermission"
"com.sun.management.internal.HotSpotThreadImpl#-[java.lang:type=Threading]"
"isInstanceOf")
...which looks correct.
Add a -Djava.security.policy=/my/all.policy
which has allPermission, and connections work OK. Removing AllPermission from
that policy, we get Exceptions again.
This looks good.
-
PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175801724
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: braces - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/67aca736..384a3a19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=16-17 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v17]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Remove import - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/597264b9..67aca736 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=15-16 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 13:58:11 GMT, Daniel Fuchs wrote: >> Kevin Walls has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - style update >> - whitespace > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java > line 1314: > >> 1312: return AccessController.doPrivileged(action, acc); >> 1313: } >> 1314: } > > This piece of code is repeated at several places in similar but different > forms - sometime we check for > `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! > SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check > for `acc == null`, sometime for `acc != null` etc.. > I wonder if we could abstract that to some utility method somewhere. Would > that make sense? Maybe that's not possible due to using sometime > `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we > could use `PrivilegedExceptionAction` everywhere at the cost of handling > `PrivilegedActionException`? > If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds > again an equivalent amount of clutter) then maybe we could at least make sure > we do the same checks in the same way (typically `acc == null` vs `acc != > null`)? Hi, We almost always check !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the RMIConnectionImpl contstructor) This keeps the "modern" case first (except in that constructor, where it is only one line for each side of the if...). This extra checking of SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the modern and old style cases distinct, based on other comments here. It keeps it simple to read I hope, but yes it is a little more verbose than it could be. I'm hoping you'll agree that as these checks will be removed anyway, probably with a release, we should delay more extensive cleanup until then. (I've also rolled back my noPermissionsACC removal to keep this change away from being a general cleanup.) I had a bunch of additional Exception handling for e.g. PrivilegedActionException I think in here, and it wasn't very pretty. But, it might look better when there are fewer nested ifs in these methods, and when we are properly in the post-SecurityManager world... 8-) Whether it checks acc != null or acc == null is based on the existing code. I think that makes this diff easier to read, but also could be reworked and be made more consistent after SM removal. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643036701
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
On Mon, 17 Jun 2024 12:33:08 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - leave noPermissionsACC in place for now
>> - leave noPermissionsACC in place for now
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1436:
>
>> 1434: return op.run();
>> 1435: } catch (Exception e) {
>> 1436: if (e instanceof RuntimeException)
>
> Enclose the next line in braces.
Yes this file is full of pre-existing style problems like that. I can update
this and its friend at 1451.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642763028
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - style update - whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/2f2179e7..597264b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=14-15 Stats: 11 lines in 2 files changed: 4 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
On Mon, 17 Jun 2024 12:33:47 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - leave noPermissionsACC in place for now
>> - leave noPermissionsACC in place for now
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 349:
>
>> 347: @SuppressWarnings("removal")
>> 348: private Subject getSubject() {
>> 349:return Subject.current();
>
> Add a leading whitespace.
got it
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642749038
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:48:14 GMT, Weijun Wang wrote: > > Does noPermissionsACC add anything? > > I don't know. My principal for this code change is that nothing is changed > for the SM-is-allowed case. I've put back the noPermissionsACC for this change, it does not have to be removed in this change. It was introduced in jdk6, and we will remove it next time we are here when we drop ACC usage. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2172948152
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - leave noPermissionsACC in place for now - leave noPermissionsACC in place for now - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/ee2dd12e..2f2179e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=13-14 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
On Sun, 16 Jun 2024 01:54:34 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Unnecessary catches to remove
>
> src/java.management/share/classes/javax/management/monitor/Monitor.java line
> 1542:
>
>> 1540: if
>> (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
>> 1541: // No SecurityManager permitted:
>> 1542: Subject.doAs(s, action); // s is permitted to be null
>
> While `s` is permitted to be null, calling `Subject.doAs(null, action)`
> actually sets the current subject to null while calling `action`. This is not
> same as directly calling `action` where the current subject (could be non
> null) is used.
Yes, good point, got it. 8-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642504749
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v14]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 844_JMX_Subject - Monitor: do not call doAs with an explicityly null Subject - Unnecessary catches to remove - Monitor update - ServerNotifForwarder simplification - Separate SM allowed and not allowed cases - ServerNotifForwarder getSubject update - Exception handling per Daniel comment - Annotation in checkAccess - Undo test policy updates - ... and 9 more: https://git.openjdk.org/jdk/compare/c0b2e87b...ee2dd12e - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/13d9202f..ee2dd12e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=12-13 Stats: 15038 lines in 603 files changed: 6231 ins; 7274 del; 1533 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:51:53 GMT, Weijun Wang wrote: >> It needs to recognise and throw RuntimeException so that a SecurityException >> isn't wrapped in a PrivilegedActionException (which gets caught by those >> blocks of code which call extractException(pe) and look at what Exception >> it contains). >> >> (test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java >> tests this) >> >> I can add a check to make sure PrivilgedActionException doesn't get wrapped. >> Daniel I think mentioned this also. > > I still don't understand how a SecurityException could be wrapped into a PAE > without these lines. I misunderstood the comment, yes I think I can remove this block now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639983304
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Unnecessary catches to remove - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/d5f68b02..13d9202f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=11-12 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:03:07 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1461:
>
>> 1459: throw rte;
>> 1460: } else {
>> 1461: throw new PrivilegedActionException(e);
>
> The 4 lines above seems unnecessary now. Plus, do not wrap
> `PrivilegedActionException` inside `PrivilegedActionException`.
It needs to recognise and throw RuntimeException so that a SecurityException
isn't wrapped in a PrivilegedActionException (which gets caught by those blocks
of code which call extractException(pe) and look at what Exception it
contains).
(test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java
tests this)
I can add a check to make sure PrivilgedActionException doesn't get wrapped.
Daniel I think mentioned this also.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639876260
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 20:52:51 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management/share/classes/javax/management/monitor/Monitor.java line
> 1543:
>
>> 1541: // No SecurityManager:
>> 1542: Subject.doAs(s, action); // s is permitted to be null
>> 1543: } else {
>
> Even though ac should never be null, I would keep the original check for ac
> == null inside the SM code path to be safe and consistent with the original
> code, and use only call doAs if allowSM is false, so like:
>
>
> if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> // No SecurityManager:
> Subject.doAs(s, action); // s is permitted to be null
> } else {
> if (ac == null) {
> throw new SecurityException("AccessControlContext cannot be null");
> }
> // ACC means SM is permitted.
> AccessController.doPrivileged(action, ac);
> }
ok done!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639833457
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v12]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Monitor update - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/242af991..d5f68b02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=10-11 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:44:17 GMT, Kevin Walls wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
>> line 353:
>>
>>> 351: } else {
>>> 352: return Subject.getSubject(AccessController.getContext());
>>> 353: }
>>
>> `Subject.current()` should work for both cases. See the impl of it.
>
> It will work to get the Subject yes.
> Do I not need the SM-enabled case, in case there some complex ACC in place?
> (combiner)
OK if it was actually getting specifically the ACC to use it maybe, but this
just needs to resolve a Subject.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639806380
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v11]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: ServerNotifForwarder simplification - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/09c2e38f..242af991 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=09-10 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:04:06 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 353:
>
>> 351: } else {
>> 352: return Subject.getSubject(AccessController.getContext());
>> 353: }
>
> `Subject.current()` should work for both cases. See the impl of it.
ok!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639766973
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:09:46 GMT, Weijun Wang wrote: > I don't quite understand why there is no more `noPermissionsACC` in > `Monotor.java`. This looks like the only behavior change when SM is allowed. > The other source change looks fine to me. Does noPermissionsACC add anything? Maybe I'm rushing the removal of ACCs where I can. Start sets acc to the current context (when SM permitted, use Subject otherwise) Set acc to noPermissionsACC when stop is called (or forget the Subject). acc is accessed within the run method. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2167949489
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 21:03:17 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1304:
>
>> 1302: // No ACC, therefore no SM. May have a Subject:
>> 1303: if (subject != null) {
>> 1304: return Subject.doAs(subject, action);
>
> Is it ever possible for acc to be `null` and `subject` not `null` and an SM
> to be enabled? Doesn't look like it, but if it ever could be, then the call
> above to `Subject.doAs` would trigger a permission check for an
> `AuthPermission("doAs")` permission.
>
> I think following Weijun's advice above is cleaner and safer, so you do one
> or the other depending on the allowSM setting, and not whether certain
> variables are null or not.
Right, the only possible assignment to acc in this file is if we were given a
Subject, and SM is permitted.
In future there will be a Subject, which can be null. While we handle SM, we
still use the ACC if RMIConnectionImpl was created with a Subject.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639610678
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 19:01:45 GMT, Weijun Wang wrote: >> Thanks I'll go through the above comments and update - some of the changes I >> see are unnecessary and from when I was trying migrating to callAs, and >> doPrivileged, and yes they can be simpler. >> >> On the allowSecurityMananger check: >> If we check in e.g. RMIConnectionImpl constructor, and if SM is allowed we >> assign an ACC. >> Later, we know if we have a non-null ACC then SM is allowed. Are you saying >> we should then check allowSecurityManager again? > > Not "then check" but "first check", as shown in my example above. I > understand you can infer whether SM is allowed by looking at the values if > `acc` and `subject`, but IMO it's not worth doing. The check is very light > and this coding style makes sure the old code and new code are cleanly > separated. It's easy to code because you don't modify existing code and only > add new code in else blocks. In the near future when we finally remove the SM > it will also be simple to remove old code. I'm trying that out in this update. It looks OK 8-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1638932678
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Separate SM allowed and not allowed cases - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/f70047a4..09c2e38f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=08-09 Stats: 52 lines in 1 file changed: 31 ins; 8 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v9]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: ServerNotifForwarder getSubject update - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/1ad97750..f70047a4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=07-08 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v8]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Exception handling per Daniel comment - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/32f52c52..1ad97750 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=06-07 Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 16:41:36 GMT, Daniel Fuchs wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1445:
>
>> 1443: } else {
>> 1444: throw new PrivilegedActionException(e);
>> 1445: }
>
> I assume there no chance that `Exception e` may already be a
> `PrivilegedActionException` here. You coud avoid casts by using instanceof
> patterns.
>
> Suggestion:
>
> if (e instanceof RuntimeException rte) {
> throw rte;
> } else {
> throw new PrivilegedActionException(e);
> }
ok yes will do that. SecurityException was another carryover from before
simplifying the updated calls, think it's no longer needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1638181708
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 20:42:27 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
> line 312:
>
>> 310: @SuppressWarnings("removal")
>> 311: final AccessControlContext acc =
>> AccessController.getContext();
>> 312: //@SuppressWarnings("removal")
>
> Remove this line?
Now the assignment to Subject is split into two alternatives, this annotation
doesn't work (without the variable declaration).
Yes I can remove this and can go with just one annotation for the method.
I am expecting to remove the deprecated methods here in a followup change after
SM.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1637973185
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v7]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Annotation in checkAccess - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/6f072338..32f52c52 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=05-06 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:55:31 GMT, Daniel Fuchs wrote: >> Hmm I may have fixed that since changing the policy files, as I'm not seeing >> the problem without that AuthPermission any more. Am just retesting >> everything before updating this... > > (Same with other policy files in which the permission was added of course) Yes these no longer seem needed. I added them in response to failures in an earlier version of the change, thanks for spotting this, I've undone the policy changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636753776
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Undo test policy updates - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/9664bef6..6f072338 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v5]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Undo test policy updates - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/422011e4..9664bef6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=03-04 Stats: 5 lines in 3 files changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:31:26 GMT, Alan Bateman wrote: >> test/jdk/javax/management/remote/mandatory/notif/policy.negative line 7: >> >>> 5: permission javax.management.MBeanPermission >>> "[domain:type=NB,name=2]", "addNotificationListener"; >>> 6: permission javax.management.MBeanPermission "*", >>> "removeNotificationListener"; >>> 7: permission javax.security.auth.AuthPermission "doAs"; >> >> I suspect that this means a doPrivileged is missing somewhere. We should not >> require the application to posess new permissions. > > I think Daniel is right, can you remove this permission and paste in the > debug output to see where this is happening? Hmm I may have fixed that since changing the policy files, as I'm not seeing the problem without that AuthPermission any more. Am just retesting everything before updating this... - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636613493
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 20:31:03 GMT, Sean Mullan wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
>> line 350:
>>
>>> 348: @SuppressWarnings("removal")
>>> 349: private Subject getSubject() {
>>> 350: Subject subject = null;
>>
>> Just call `Subject.current()`. When SM is allowed, it is equivalent to
>> `getSubject(AccessController.getContext())`.
>
> For the same reason you should be able to just call `Subject.current` in the
> tests. I don't think you need the `SimpleStandard.useGetSubjectACC` property
> to toggle the testing of either old or replacement APIs as long as the test
> has runs in both SM allow and disallow (i.e., no SM prop set) modes. Would
> you agree Weijun?
I thought there was a comment elsewhere suggesting using SM=allow to test with
Subject.getSubject() and Subject.current(), and when SM not allowed, to only
use Subject.current() ?
That was the reason for the property in the tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636536113
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 16:55:44 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1633:
>
>> 1631: }
>> 1632: } else {
>> 1633: // ACC is present, we have a Subject and SM is
>> permitted:
>
> While extract the `action` variable? The old code on lines 1590-1592 has no
> problem.
OK I can make this more like the original.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636515151
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: udpates - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/56f9111e..422011e4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=02-03 Stats: 35 lines in 2 files changed: 6 ins; 23 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 16:53:09 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1438:
>
>> 1436: return
>> AccessController.doPrivileged((PrivilegedExceptionAction) () ->
>> Subject.doAs(subject, op), acc);
>> 1437: }
>> 1438: } catch (PrivilegedActionException pe) {
>
> What is this catch block for? The cause of a `PrivilegedActionException`
> should not be a unchecked exception.
Looks like the catch of PrivilegedActionException is no longer necessary, will
be removed.
Was from from previously using a different doAs or call which needed it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636417022
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 20:58:54 GMT, Sean Mullan wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
>> line 1436:
>>
>>> 1434: } else {
>>> 1435: // ACC is present, we have a Subject and SM is
>>> permitted:
>>> 1436: return
>>> AccessController.doPrivileged((PrivilegedExceptionAction) () ->
>>> Subject.doAs(subject, op), acc);
>>
>> Why is it necessary to call both `doAs` and `doPrivileged`?
>
> Can you just call `AccessController.doPrivileged(op, acc)` as in the original
> code? `Subject.doAs` requires a permission, which is why I assume you are
> wrapping it in a `doPrivileged` but you are asserting all of the permissions
> of the `java.management` module, which means you probably want to use limited
> doPriv and only assert the `getSubject` permission but that is getting
> complicated. I'd go back to the original call as it should work in the SM
> allow case.
Yes, this can be simpler, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636417433
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 16:59:31 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management/share/classes/javax/management/monitor/Monitor.java line
> 1541:
>
>> 1539: if (ac == null) {
>> 1540: // No SecurityManager:
>> 1541: Subject.doAs(s, action);
>
> What if `s` is null?
Subject.doAs accepts a null Subject, so that's OK?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636324060
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Remove annotations - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/58892d76..ed386505 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
On Tue, 11 Jun 2024 17:03:58 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1301:
>
>> 1299: }
>> 1300: };
>> 1301: if (acc == null) {
>
> This is a comment to all the code changes in this PR. I would recommend we
> make use of the `allowSecurityManager()` call everywhere when the behavior is
> different, like this:
>
> if (SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> if (acc == null)
> return action.run();
> else
> return AccessController.doPrivileged(action, acc);
> } else {
> if (subject == null)
> return action.run();
> else
> return Subject.doAs(subject, action);
> }
>
> This makes me much more confident.
Thanks I'll go through the above comments and update - some of the changes I
see are unnecessary and from when I was trying migrating to callAs, and
doPrivileged, and yes they can be simpler.
On the allowSecurityMananger check:
If we check in e.g. RMIConnectionImpl constructor, and if SM is allowed we
assign an ACC.
Later, we know if we have a non-null ACC then SM is allowed. Are you saying we
should then check allowSecurityManager again?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635291389
Re: RFR: 8327793: Deprecate jstatd for removal [v3]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - Replace (C) - Remove annotations - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/72ac8818..58892d76 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=01-02 Stats: 24 lines in 9 files changed: 0 ins; 16 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8327793: Deprecate jstatd for removal [v2]
On Tue, 11 Jun 2024 16:54:36 GMT, Kevin Walls wrote: >> jstatd is an RMI server application which monitors HotSpot VMs, and provides >> an interface to the monitoring tool jstat, for use across a remote RMI >> connection. >> >> RMI is not how modern applications communicate. It is an old transport with >> long term security concerns, and configuration difficulties with firewalls. >> >> The jstatd tool should be removed. Deprecating and removing jstatd will not >> affect usage of jstat for monitoring local VMs using the Attach API. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > No annotations for src/jdk.jstatd/share/classes/sun/jvmstat/monitor/remote OK sure - I had thought the src annotations were useful but if not needed I am taking them out. Yes, I was thinking to do a separate man page CR. - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2161244463
Re: RFR: 8327793: Deprecate jstatd for removal
On Tue, 11 Jun 2024 13:35:19 GMT, Alan Bateman wrote: > The sun.jvmstat.monitor.remote package is not exported so I don't think > adding `@Deprecated` makes sense. Sure, happy to not add annotations in sun.jvmstat.monitor.remote (RemoteHost.java, RemoteVm.java). - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2161212800
Re: RFR: 8327793: Deprecate jstatd for removal [v2]
> jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: No annotations for src/jdk.jstatd/share/classes/sun/jvmstat/monitor/remote - Changes: - all: https://git.openjdk.org/jdk/pull/19658/files - new: https://git.openjdk.org/jdk/pull/19658/files/60bde969..72ac8818 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=00-01 Stats: 5 lines in 2 files changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Sean comments - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/d43f9a34..56f9111e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=01-02 Stats: 5 lines in 2 files changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v2]
On Tue, 11 Jun 2024 14:02:17 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> More consistent style of calls and comments.
>
> test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
> line 36:
>
>> 34: * @run main NotificationEmissionTest 1
>> 35:
>> 36: * @run main/othervm NotificationEmissionTest 1
>
> Lines 34 & 36: is it necessary to run NotificationEmissionTest twice with and
> w/o `othervm`?
It's unnecessary, thanks.
> test/jdk/javax/management/remote/mandatory/passwordAuthenticator/SimpleStandard.java
> line 155:
>
>> 153: */
>> 154: private void checkSubject() {
>> 155: Subject subject =
>> Boolean.getBoolean("SimpleStandard.useGetSubjectACC") ?
>
> Nit, remove extra leading space.
got it
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635157731
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635157877
RFR: 8327793: Deprecate jstatd for removal
jstatd is an RMI server application which monitors HotSpot VMs, and provides an interface to the monitoring tool jstat, for use across a remote RMI connection. RMI is not how modern applications communicate. It is an old transport with long term security concerns, and configuration difficulties with firewalls. The jstatd tool should be removed. Deprecating and removing jstatd will not affect usage of jstat for monitoring local VMs using the Attach API. - Commit messages: - Remove tmp file - Annotations - Annotations - Merge remote-tracking branch 'upstream/master' into 8327793_jstatd_deprecate - Basic deprecation of module, jstatd prints warning. Changes: https://git.openjdk.org/jdk/pull/19658/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19658&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327793 Stats: 35 lines in 12 files changed: 23 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19658.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19658/head:pull/19658 PR: https://git.openjdk.org/jdk/pull/19658
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v2]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: More consistent style of calls and comments. - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/22424a8e..d43f9a34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=00-01 Stats: 12 lines in 1 file changed: 5 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Mon, 10 Jun 2024 14:28:25 GMT, Alan Bateman wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1634:
>
>> 1632: } else {
>> 1633: // We have ACC therefore SM is permitted, and
>> Subject must be non-null:
>> 1634: return Subject.doAs(subject,
>> (PrivilegedExceptionAction) () ->
>> AccessController.doPrivileged((PrivilegedExceptionAction) () ->
>> wrappedClass.cast(mo.get()), acc));
>
> This is not readable. If you create the PAEs explicitly then the casts will
> go away and it will be much easier to read.
Yes will simplify this!
This style was actually from the idea of moving to Subject.callAs, and nesting
calls to keep the doPrivileged, but I don't think it will be necessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1633455244
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Mon, 10 Jun 2024 11:28:28 GMT, Kevin Walls wrote: > JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. When SecurityManager is not permitted, which is the default, use the Subject.current() call. If SM is permitted, due to -Djava.security.manager=allow then the old Subject.getSubject(AccessController.getContext()) call is used. Tests are updated to not require -Djava.security.manager=allow and will test with and without that setting. Also additionally update tests to use Subject.current(), but also have a setting to test the old Subject.getSubject(AccessController.getContext()) call with -Djava.security.manager=allow (see ThreadPoolAccTest and test/jdk/javax/management/remote/mandatory/passwordAuthenticator). - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2158515271
RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
JMX uses APIs related to the Security Mananger which are deprecated. Use of AccessControlContext will be removed when Security Manager is removed. Until then, updates are needed to not require setting -Djava.security.manager=allow to use JMX authentication. - Commit messages: - Additional test combination, old getSubject call with sm=allow - Test updates - whitespace - Merge remote-tracking branch 'upstream/master' into 844_JMX_Subject - 844: JMX attaching of Subject does not work when security manager not allowed Changes: https://git.openjdk.org/jdk/pull/19624/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=00 Issue: https://bugs.openjdk.org/browse/JDK-844 Stats: 160 lines in 19 files changed: 109 ins; 10 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]
On Wed, 24 Apr 2024 10:50:44 GMT, Doug Simon wrote: >> This PR adds a check for the format of ProblemList files and ensures they >> only have entries referring to existing tests. >> >> The cleanups in the second commit of this PR were done based on the output >> of `CheckProblemLists`: >> >>> make test TEST=build/problemLists/CheckProblemLists.java >> ... >> STDOUT: >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt >> Checked 13 problem list files >> Test roots: >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk >> /Users/dnsimon/dev/jdk-jdk/open/test/lib-test >> /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test >> /Users/dnsimon/dev/jdk-jdk/open/test/jaxp >> /Users/dnsimon/dev/jdk-jdk/open/test/langtools >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg >> Following errors found: >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: >> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under >> any test root >> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all >> >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: >> TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, >> issueId=000] duplicates >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76 >> java/util/Properties/StoreReproducibilityTest.java 000 generic-all >> >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: >> java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any >> test root >> java/lang/management/MemoryMXBean/PendingAllGC.sh 8158837 >> generic-all >> >> ... > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > removed CheckProblemLists.java The tidyup looks good! I don't understand that this is titled as JDK-8330755 but that's already integrated. So this needs to be done in a separate JBS entry and if the suggested CheckProblemLists.java is not going to be in it, we remove that from the description. - PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2108075635
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 14:44:29 GMT, Weijun Wang wrote: >> Right, this does not depend on the SM. All we need to do is get the >> Subject. >> This method implements the basic monitor (readonly) and control (readwrite) >> access. >> accessMap maps identity String to Access, and the checkAccess() method here >> will check the Subject by using of its Principal names as keys in that map. > > Do you know where the subject is set? If it's set by a `doAs` call then it > will co-operate with `current()` no matter if SM is allowed. I tried to > search in the whole module and cannot find a `doAs` call. If it is also > through `SubjectDomainCombiner` then it only works with SM. Subject is stored in the RMIConnectionImpl: src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java (That is complicated by SubjectDelegation, which we deprecated for removal. I have the PR out to remove it: https://github.com/openjdk/jdk/pull/18025 ) makeClient in RMIJRMPServerImpl creates RMIConnectionImpl ..and RMIServerImpl.java has a doNewClient method calling that. This is what takes a Credentials Object and deals withJMXAuthenticator to get an authenticated Subject. None of this requires the SM. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1513164360
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 19:57:25 GMT, Sean Mullan wrote: >> I was not exactly sure if we will support this functionality when there is >> no SM. The class name has `AccessControler` and the method names use >> `checkAccess`, but they actually do not always depend on security manager. > > I think we need @kevinjwalls or @dfuch to help advise on this. Right, this does not depend on the SM. All we need to do is get the Subject. This method implements the basic monitor (readonly) and control (readwrite) access. accessMap maps identity String to Access, and the checkAccess() method here will check the Subject by using of its Principal names as keys in that map. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1512676642
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v3]
On Fri, 26 Jan 2024 21:06:00 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix nullptr only contained in strings. That is long. Hypnotic to read. One very trivial nit, use or ignore as required, but all good that I can see. - Marked as reviewed by kevinw (Committer). PR Review: https://git.openjdk.org/jdk/pull/17593#pullrequestreview-1847074612
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v3]
On Fri, 26 Jan 2024 21:06:00 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix nullptr only contained in strings. test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTest/libVThreadTest.cpp line 426: > 424: } > 425: > 426: // #5: Test JVMTI GetLocalObject function with nullptr value_ptr "with null value_ptr" ? - PR Review Comment: https://git.openjdk.org/jdk/pull/17593#discussion_r1468449036
Withdrawn: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 21 Nov 2023 17:57:32 GMT, Kevin Walls wrote: > RMI Connections (in general) should use a timeout on the Socket connect call > by default. > > JMX connections use RMI and some connection failures never terminate. The > hang described in 8316649 is hard to reproduce manually: the description says > it can be caused by a firewall that never returns a response. > > src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java > has other timeouts but nothing to control the initial Socket connect. > > Defaulting to a 1 minute timeout on connect has no effect on existing tests > for RMI and JMX, and should go unnoticed in applications unless there really > is a significant connection delay. Specifying zero for the new System > Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path > of a connect with no timeout. > > I have a test, but it is not realistically usable: although specifying a 1 > millisecond timeout will often fail (as expected/desired for the test), it > will often pass as the connection happens immediately. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16771
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 21 Nov 2023 17:57:32 GMT, Kevin Walls wrote: > RMI Connections (in general) should use a timeout on the Socket connect call > by default. > > JMX connections use RMI and some connection failures never terminate. The > hang described in 8316649 is hard to reproduce manually: the description says > it can be caused by a firewall that never returns a response. > > src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java > has other timeouts but nothing to control the initial Socket connect. > > Defaulting to a 1 minute timeout on connect has no effect on existing tests > for RMI and JMX, and should go unnoticed in applications unless there really > is a significant connection delay. Specifying zero for the new System > Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path > of a connect with no timeout. > > I have a test, but it is not realistically usable: although specifying a 1 > millisecond timeout will often fail (as expected/desired for the test), it > will often pass as the connection happens immediately. Thanks for the feedback. Additionally, there can be a concern that affecting all RMI activity with the timeout property could produce unintended side-effects. At the moment the demand for the timeout is not strong, and the behaviour is what it has been for many years, so I am going to close this without progressing it. - PR Comment: https://git.openjdk.org/jdk/pull/16771#issuecomment-1835814563
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 21 Nov 2023 17:57:32 GMT, Kevin Walls wrote: > RMI Connections (in general) should use a timeout on the Socket connect call > by default. > > JMX connections use RMI and some connection failures never terminate. The > hang described in 8316649 is hard to reproduce manually: the description says > it can be caused by a firewall that never returns a response. > > src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java > has other timeouts but nothing to control the initial Socket connect. > > Defaulting to a 1 minute timeout on connect has no effect on existing tests > for RMI and JMX, and should go unnoticed in applications unless there really > is a significant connection delay. Specifying zero for the new System > Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path > of a connect with no timeout. > > I have a test, but it is not realistically usable: although specifying a 1 > millisecond timeout will often fail (as expected/desired for the test), it > will often pass as the connection happens immediately. It may be that changing the base RMI implementation isn't desirable. I am bringing up a different implementation, using a new ClientSocketFactory for JMX which implements the connect timeout. So a new JMX-specific property, not a general RMI property, and we limit the impact of the change, so it won't affect any other RMI apps. I'm putting this in https://github.com/openjdk/jdk/pull/16856 it's in draft right now, I'll complete the details and see if it is a better way forward (will close this PR if the new one is better). - PR Comment: https://git.openjdk.org/jdk/pull/16771#issuecomment-1830309421
