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

[Ulf Zibis]

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