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

[Michael McMahon]

Ulf,

It seems the NameComparator class uses exactly the same algorithm as the
CASE_INSENSITIVE_ORDER comparator provided by the String class. So,
it probably makes sense to replace NameComparator with the one from String
(one less class in rt.jar :) )

On "SystemRoot" vs "systemroot", I had thought that Windows required the 
sort
to be case-sensitive. But, I double-checked and it doesn't. So we can 
use "SystemRoot".

I think your original comment about not needing the check for sb being 
empty was correct.
On Windows, we are ensuring that SystemRoot will at a minimum always be set.

I'll send another webrev later reflecting these and David's comments.

- Michael.

Ulf Zibis wrote:
> You can have the code much shorter (and faster):
>
>     // Only for use by ProcessImpl.start()
>     String toEnvironmentBlock() {
>         // Sort Unicode-case-insensitively by name
>         Map.Entry<String,String>[] list = entrySet().toArray(new 
> Map.Entry<>[size()]);
>         Arrays.sort(list, entryComparator);
>
>         StringBuilder sb = new StringBuilder(list.length*30);
>         for (int i = 0, cmp = -1; ; i++) {
>             Map.Entry<String,String> e = i < list.length ? list[i] : 
> null;
>             final String ROOT = "SystemRoot";
>             if (cmp < 0 && (e == null || (cmp = 
> nameComparator.compare(e.getKey(), ROOT)) > 0)) {
>                 String value = getenv(ROOT);
>                 if (value != null)
>                     addEnv(sb, ROOT, value);
>                 // Ensure double NUL termination, even if environment 
> is empty.
>                 else if (list.isEmpty())
>                     sb.append('\u0000');
>             }
>             if (e == null)  break;
>             addEnv(sb, e.getKey(), e.getValue());
>         }
>         sb.append('\u0000');
>         return sb.toString();
>     }
>
>     // add the environment variable to the child
>     private static void addEnv(StringBuilder sb, String key, String 
> value) {
>         sb.append(key+"="+value+'\u0000'); // under the hood, it 
> compiles to 4 sb.append()
>     }
>
> Do you like it?
>
> Note: I think, if we use NameComparator, we can use "SystemRoot".
>
> -Ulf
>
>
> Am 14.04.2011 16:06, schrieb Michael McMahon:
>> An updated webrev is available for this fix. I'll probably go ahead
>> with the CCC request for the spec. change anyway.
>>
>> http://cr.openjdk.java.net/~michaelm/7034570/webrev.2/
>>
>>
>> Thanks,
>> Michael.
>>