Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[Michael McMahon]

Ulf,

Thanks for the comments. I think I will remove the @SuppressWarning for now,
and we can look at generifying CheckedEntrySet another time.

The environment block is required to be sorted when you call CreateProcess()
on Windows.

Yes,  as per the earlier message to David Holmes, I'm looking at 
iterating once
over the list (rather than the Set).

Lastly, even though environment variables are case-insensitive on 
Windows, it
does "remember" the case that was used (rather than normalising the 
names to uppercase
or whatever). Java has always treated them as case-sensitive. So, I 
don't think
we can convert the names to uppercase.

- Michael.

On 13/04/2011 15:20, Ulf Zibis wrote:
> First: please add some more description to the subject line.
>
> ProcessEnvironment :
>
> Line 146: There is a superfluous space between 'checkedEntry' and 
> '(Object o)'.
>
> Instead of suppressing the warnings, we could code:
>         public boolean contains(Map.Entry<String,String> o) {return 
> s.contains(checkedEntry(o));}
>         public boolean remove(Map.Entry<String,String> o)   {return 
> s.remove(checkedEntry(o));}
>
> Maybe we could generify the whole inner class against CheckedEntry:
>     private static class CheckedEntrySet extends 
> AbstractSet<CheckedEntry> {
>
> I don't know why we need the environment block sorted, but I think, we 
> could use a SortedSet from the beginning instead of sorting a normal 
> Set later.
>
> Iterating over the List list should be faster than iterating over the 
> Set entries to find the "SystemRoot".
> Additionally I guess, TreeSet should be faster than HashSet for such 
> few entries.
>
> Anyway, what about:
>  305         EnsureSystemRoot: {
>  306             final String SR = "SystemRoot";
>  307             for (Map.Entry<String,String> entry : list) {
>  308                 if (entry.getKey().equalsIgnoreCase(SR))
>  309                     break EnsureSystemRoot;
>  310             }
>  311             list.add(new 
> AbstractMap.SimpleEntry<String,String>(SR, getenv(SR)));
>  312         }
>  318
>
> We do not have to iterate twice, to find the "SystemRoot". We could 
> insert the missing value while filling the StringBuilder
>
> If we would generally uppercase the entries by put(), we wouldn't have 
> to scan each entry by equalsIgnoreCase() to find the "SystemRoot". We 
> simply could use entries.contains("SYSTEMROOT")
>
> -Ulf
>
>
> Am 13.04.2011 13:33, schrieb Michael McMahon:
>> 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,
>> Michael.
>>