Hi Martin,
I have restructured the processReaperExecutor construction. It now
incorporates system thread group search and thread pool construction in
one doPrivileged call. I also extracted the creation of ThreadFactory
into a local variable so it's more explicit now. Here's the webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.09/
...java/lang/ProcessBuilder tests still pass.
Regards, Peter
P.S. I don't belive Executors.newCachedTreadPool() could strengthen
security in future, since this would break existing user code. The
javadoc does not specify any SecurityExceptions. But anyway - wrapping
the whole logic of processReaperExecutor construction in one
doPrivileged call allowed me to use local variables instead of private
static final fields for systemThreadGroup and threadFactory, so this
looks nicer too.
On 05/06/2014 07:41 PM, Martin Buchholz wrote:
On Sun, May 4, 2014 at 3:26 AM, Peter Levart <peter.lev...@gmail.com
<mailto:peter.lev...@gmail.com>> wrote:
Ah, I've forgotten to mention the most important change from
webrev.07.
Original code wraps the processReaperExecutor construction into
the doPrivileged() call. I think this was not needed.
The Executors.newCachedThreadPool() does not need any special
privileges. And construction of nested class
ProcessReaperThreadFactory also didn't need any special privileges
- apart from static initialization which called the
getRootThreadGroup() method. But that method did it's own
doPrivileged() wrapping in order to be able to climb the
ThreadGroup hierarchy (which requires privilege). In webrev.07 I
followed original code and wrapped the construction of a
processReaperExecutor into a doPrivileged() although in webrev.07
the rootThreadGroup search happens as part of UNIXProcess static
initialization and is already wrapped in doPrivileged(). In
webrev.08 I removed this superfluous doPrivileged(). I think this
is correct. SecurityManagerClinit test passes.
Although I think you're right with the current implementation, it
seems too brittle to me to remove the doPrivileged, since you have no
idea what future changes to newCachedThreadPool() might do (or what
other JDK implementations might do); notably it might instantiate some
kind of micro-manager thread or do some other privileged operation.
If you want to have only one call to doPrivileged, you can cause both
rootThreadGroup and processReaperExecutor to be initialized in one
static block, although you will need to jump through some hoops to
keep these fields final.
---
As it stands,
Executors.newCachedThreadPool(grimReaper -> {
doesn't mention either the type ThreadFactory or Runnable.
It might be clearer (more verbosity in the spirit of Java) if you used
explicit ThreadFactory type
ThreadFactory factory = grimReaper -> ...
