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

[Martin Buchholz]

Hi Peter,

Your last version version looks very good.  Approved!  I'm reduced to
asking you to fix ancient buglets of mine.

*"I'd spell creat with an e.*"



 188      * Create a process. Depending on the mode flag, this is done by

*"I'd spell Create with an s.*"







On Wed, May 7, 2014 at 12:42 AM, Peter Levart <peter.levart at gmail.com>wrote:

>  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>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 -> ...
>
>
>