Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Sat, 26 Nov 2022 17:24:02 GMT, Alan Bateman wrote: > Not important but can eliminate the casts from privilegedThreadSetXXX methods > if you separate the creation of the PrivilegedAction from the doPriv call. I chose to keep it as is since there was already another doPriv in the file that uses the cast style. - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Sat, 26 Nov 2022 15:55:27 GMT, Ryan Ernst wrote: >> This commit guards thread modifications for the process reaper thread with >> doPrivileged. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > Revert factory method This looks okay. Not important but can eliminate the casts from privilegedThreadSetXXX methods if you separate the creation of the PrivilegedAction from the doPriv call. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Wed, 23 Nov 2022 16:02:37 GMT, Chris Hegarty wrote: >> I would prefer to to avoid creating new factories when the desired function >> can be done on the resulting thread. >> Such as `setDaemon()` and `setName()`, etc. >> It does avoid the doPriv in this case, but is not necessary and when the >> security manager goes away, will leave around clutter (duplicated) >> functionality. > > Looking beyond this specific change, there is a lot of potential use for this > new factory elsewhere in the code. It also avoids similar bugs from possibly > reoccurring (by having the setDaemon call inside the factory). In the interest of making progress, let’s revert the change to the factory. This can be done separately, if at all. - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
On Sat, 26 Nov 2022 15:50:54 GMT, Ryan Ernst wrote: >> This commit guards thread modifications for the process reaper thread with >> doPrivileged. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > Revert factory method LGTM - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]
> This commit guards thread modifications for the process reaper thread with > doPrivileged. Ryan Ernst has updated the pull request incrementally with one additional commit since the last revision: Revert factory method - Changes: - all: https://git.openjdk.org/jdk/pull/11309/files - new: https://git.openjdk.org/jdk/pull/11309/files/a822cc8e..bc42d415 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11309&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11309&range=01-02 Stats: 29 lines in 2 files changed: 9 ins; 12 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/11309.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11309/head:pull/11309 PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v2]
> This commit guards thread modifications for the process reaper thread with > doPrivileged. Ryan Ernst has updated the pull request incrementally with two additional commits since the last revision: - Merge pull request #1 from ChrisHegarty/reaper_thread_modify Add privileged helper method and update existing test - Add privileged helper method and update existing test - Changes: - all: https://git.openjdk.org/jdk/pull/11309/files - new: https://git.openjdk.org/jdk/pull/11309/files/c02f3f09..a822cc8e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11309&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11309&range=00-01 Stats: 31 lines in 2 files changed: 20 ins; 6 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/11309.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11309/head:pull/11309 PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 15:22:47 GMT, Roger Riggs wrote: >> src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java line 137: >> >>> 135: public static Thread newSystemThread(String name, Runnable target, >>> 136: long stackSize, int priority, >>> 137: boolean daemon) { >> >> Thanks for adding this overload, I think that it will be useful for the >> future too. ( it never seems to matter how many variants of these >> factories we have, we still need one more :-) ) > > I would prefer to to avoid creating new factories when the desired function > can be done on the resulting thread. > Such as `setDaemon()` and `setName()`, etc. > It does avoid the doPriv in this case, but is not necessary and when the > security manager goes away, will leave around clutter (duplicated) > functionality. Looking beyond this specific change, there is a lot of potential use for this new factory elsewhere in the code. It also avoids similar bugs from possibly reoccurring (by having the setDaemon call inside the factory). - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 08:38:02 GMT, Chris Hegarty wrote: >> This commit guards thread modifications for the process reaper thread with >> doPrivileged. > > src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java line 137: > >> 135: public static Thread newSystemThread(String name, Runnable target, >> 136: long stackSize, int priority, >> 137: boolean daemon) { > > Thanks for adding this overload, I think that it will be useful for the > future too. ( it never seems to matter how many variants of these factories > we have, we still need one more :-) ) I would prefer to to avoid creating new factories when the desired function can be done on the resulting thread. Such as `setDaemon()` and `setName()`, etc. It does avoid the doPriv in this case, but is not necessary and when the security manager goes away, will leave around clutter (duplicated) functionality. - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 05:01:40 GMT, Ryan Ernst wrote: > This commit guards thread modifications for the process reaper thread with > doPrivileged. Changes requested by chegar (Reviewer). src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java line 137: > 135: public static Thread newSystemThread(String name, Runnable target, > 136: long stackSize, int priority, > 137: boolean daemon) { Thanks for adding this overload, I think that it will be useful for the future too. ( it never seems to matter how many variants of these factories we have, we still need one more :-) ) - PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
On Wed, 23 Nov 2022 05:01:40 GMT, Ryan Ernst wrote: > This commit guards thread modifications for the process reaper thread with > doPrivileged. Hi @rjernst Thanks for taking this one on. I agree with adding the privileged blocks around the calls to Thread::setName, but the usage raises a warning which fails the build. It might be cleaner and easier to refactor into a privilegedThreadSetName helper method. Additionally, I noticed that there is an existing test that can be modified slightly to cover this. I've put these two comments / suggestions in the form of a PR and raised it against your branch. https://github.com/rjernst/jdk/pull/1 - PR: https://git.openjdk.org/jdk/pull/11309
RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
This commit guards thread modifications for the process reaper thread with doPrivileged. - Commit messages: - 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread Changes: https://git.openjdk.org/jdk/pull/11309/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11309&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297451 Stats: 31 lines in 2 files changed: 19 ins; 1 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/11309.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11309/head:pull/11309 PR: https://git.openjdk.org/jdk/pull/11309