Re: RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread [v3]

[Ryan Ernst]

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]

[Alan Bateman]

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]

[Chris Hegarty]

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]

[Chris Hegarty]

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]

[Ryan Ernst]

> 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=11309=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=11309=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]

[Ryan Ernst]

> 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=11309=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=11309=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

[Roger Riggs]

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

[Chris Hegarty]

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

[Chris Hegarty]

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

[Ryan Ernst]

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