Review request for 7034570

[Michael McMahon]

Alan Bateman wrote:
> Michael McMahon wrote:
>> This issue occurs on some versions of Windows since we switched compiler
>> to VC 2010. The new C runtime library requires the "SystemRoot" 
>> environment
>> variable to be set, when launching sub-processes via Runtime.exec() 
>> or ProcessBuilder.
>>
>> The problem occurs in instances where the environment is specified by 
>> the caller,
>> rather than simply inheriting (or inheriting and adding to) the 
>> environment of the parent
>> process.
>>
>> The change is simple enough. We just check for the presence of 
>> "SystemRoot" in the
>> child process's environment, and set it if it's not there.  No change 
>> will be visible
>> to the parent process. But, to avoid ambiguity, we'd like to make the 
>> change explicit
>> with a small spec. change.
>>
>> http://cr.openjdk.java.net/~michaelm/7034570/webrev.1/
> Thanks for taking this one. Although we might be seeing this issue 
> more frequently since moving to the newer C runtime, I'm pretty sure 
> that not having SystemRoot in the environment has always lead to an 
> altered state of consciousness. At one point I recall that Windows 
> Sockets could be initialized but would fail creating sockets when this 
> variable wasn't set, something to do with how layered service 
> providers were located via the registry if I recall.
>
> Anyway, it's good to get this fixed. The update to the javadoc mostly 
> looks good to me. One suggestion for ProcessBuilder.start is that 
> "beyond those in the specified environment" doesn't quite work as the 
> environment isn't specified to the method. How about "beyond those in 
> the process builder's {@link #environment()}". The update to 
> Runtime.exec looks good to me.
>
Ok.
> In toEnvironmentBlock does the getenv("SystemRoot") need to be done in 
> a privileged block (I'm just thinking of the case where you have 
> permissions to exec the process but not read the variable). Also do 
> you need to handle the case that it is null?
>
The permission check happens at a higher level. We're dealing directly 
with the data at this level.
So, we don't need a privileged block.

Yes, the null case needs to be distinguished.

I'll send another webrev soon.

Thanks,
Michael.