7034570: java.lang.Runtime.exec(String[] cmd, String[] env) can not work...

7034570: java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Reviewed-by: dholmes, alanb

src/share/classes/java/lang/ProcessBuilder.java

@@ -938,6 +938,11 @@ public final class ProcessBuilder
      * but at the very least the command must be a non-empty list of
      * non-null strings.
      *
+     * <p>A minimal set of system dependent environment variables may
+     * be required to start a process on some operating systems.
+     * As a result, the subprocess may inherit additional environment variable
+     * settings beyond those in the process builder's {@link #environment()}.
+     *
      * <p>If there is a security manager, its
      * {@link SecurityManager#checkExec checkExec}
      * method is called with the first component of this object's

src/share/classes/java/lang/Runtime.java

@@ -544,6 +544,11 @@ public class Runtime {
      * <p>If <tt>envp</tt> is <tt>null</tt>, the subprocess inherits the
      * environment settings of the current process.
      *
+     * <p>A minimal set of system dependent environment variables may
+     * be required to start a process on some operating systems.
+     * As a result, the subprocess may inherit additional environment variable
+     * settings beyond those in the specified environment.
+     *
      * <p>{@link ProcessBuilder#start()} is now the preferred way to
      * start a process with a modified environment.
      *

src/windows/classes/java/lang/ProcessEnvironment.java

@@ -143,7 +143,7 @@ final class ProcessEnvironment extends HashMap<String,String>
                 public void remove() { i.remove();}
             };
         }
-        private static Map.Entry<String,String> checkedEntry (Object o) {
+        private static Map.Entry<String,String> checkedEntry(Object o) {
             Map.Entry<String,String> e = (Map.Entry<String,String>) o;
             nonNullString(e.getKey());
             nonNullString(e.getValue());
@@ -285,7 +285,7 @@ final class ProcessEnvironment extends HashMap<String,String>
         return (Map<String,String>) theEnvironment.clone();
     }
 
-    // Only for use by Runtime.exec(...String[]envp...)
+    // Only for use by ProcessBuilder.environment(String[] envp)
     static Map<String,String> emptyEnvironment(int capacity) {
         return new ProcessEnvironment(capacity);
     }
@@ -299,19 +299,46 @@ final class ProcessEnvironment extends HashMap<String,String>
         Collections.sort(list, entryComparator);
 
         StringBuilder sb = new StringBuilder(size()*30);
-        for (Map.Entry<String,String> e : list)
-            sb.append(e.getKey())
-              .append('=')
-              .append(e.getValue())
-              .append('\u0000');
-        // Ensure double NUL termination,
-        // even if environment is empty.
-        if (sb.length() == 0)
+        int cmp = -1;
+
+        // Some versions of MSVCRT.DLL require SystemRoot to be set.
+        // So, we make sure that it is always set, even if not provided
+        // by the caller.
+        final String SYSTEMROOT = "SystemRoot";
+
+        for (Map.Entry<String,String> e : list) {
+            String key = e.getKey();
+            String value = e.getValue();
+            if (cmp < 0 && (cmp = nameComparator.compare(key, SYSTEMROOT)) > 0) {
+                // Not set, so add it here
+                addToEnvIfSet(sb, SYSTEMROOT);
+            }
+            addToEnv(sb, key, value);
+        }
+        if (cmp < 0) {
+            // Got to end of list and still not found
+            addToEnvIfSet(sb, SYSTEMROOT);
+        }
+        if (sb.length() == 0) {
+            // Environment was empty and SystemRoot not set in parent
             sb.append('\u0000');
+        }
+        // Block is double NUL terminated
         sb.append('\u0000');
         return sb.toString();
     }
 
+    // add the environment variable to the child, if it exists in parent
+    private static void addToEnvIfSet(StringBuilder sb, String name) {
+        String s = getenv(name);
+        if (s != null)
+            addToEnv(sb, name, s);
+    }
+
+    private static void addToEnv(StringBuilder sb, String name, String val) {
+        sb.append(name).append('=').append(val).append('\u0000');
+    }
+
     static String toEnvironmentBlock(Map<String,String> map) {
         return map == null ? null :
             ((ProcessEnvironment)map).toEnvironmentBlock();

test/java/lang/ProcessBuilder/Basic.java

@@ -26,7 +26,7 @@
  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 4986689
  *      5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
  *      6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
- *      4947220 7018606
+ *      4947220 7018606 7034570
  * @summary Basic tests for Process and Environment Variable code
  * @run main/othervm/timeout=300 Basic
  * @author Martin Buchholz
@@ -1440,11 +1440,12 @@ public class Basic {
         // Check for sort order of environment variables on Windows.
         //----------------------------------------------------------------
         try {
+            String systemRoot = "SystemRoot=" + System.getenv("SystemRoot");
             // '+' < 'A' < 'Z' < '_' < 'a' < 'z' < '~'
             String[]envp = {"FOO=BAR","BAZ=GORP","QUUX=",
-                            "+=+", "_=_", "~=~"};
+                            "+=+", "_=_", "~=~", systemRoot};
             String output = nativeEnv(envp);
-            String expected = "+=+\nBAZ=GORP\nFOO=BAR\nQUUX=\n_=_\n~=~\n";
+            String expected = "+=+\nBAZ=GORP\nFOO=BAR\nQUUX=\n"+systemRoot+"\n_=_\n~=~\n";
             // On Windows, Java must keep the environment sorted.
             // Order is random on Unix, so this test does the sort.
             if (! Windows.is())
@@ -1452,6 +1453,21 @@ public class Basic {
             equal(output, expected);
         } catch (Throwable t) { unexpected(t); }
 
+        //----------------------------------------------------------------
+        // Test Runtime.exec(...envp...)
+        // and check SystemRoot gets set automatically on Windows
+        //----------------------------------------------------------------
+        try {
+            if (Windows.is()) {
+                String systemRoot = "SystemRoot=" + System.getenv("SystemRoot");
+                String[]envp = {"FOO=BAR","BAZ=GORP","QUUX=",
+                                "+=+", "_=_", "~=~"};
+                String output = nativeEnv(envp);
+                String expected = "+=+\nBAZ=GORP\nFOO=BAR\nQUUX=\n"+systemRoot+"\n_=_\n~=~\n";
+                equal(output, expected);
+            }
+        } catch (Throwable t) { unexpected(t); }
+
         //----------------------------------------------------------------
         // System.getenv() must be consistent with System.getenv(String)
         //----------------------------------------------------------------