RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

[Peter Levart]

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.levart at gmail.com 
> <mailto:peter.levart at 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 -> ...