Windows, Starlark: fix command line arg quoting

Fix the quoting logic for command line arguments
passed to CreateProcessW.

Related to bazelbuild#7314

Fixes bazelbuild#7122

src/main/java/com/google/devtools/build/lib/shell/ShellUtils.java

@@ -142,4 +142,86 @@ public static void tokenize(List<String> options, String optionString)
     }
   }
 
+  /** Escape command line arguments for {@code CreateProcessW} on Windows. */
+  public static String escapeCreateProcessArg(String s) {
+    if (s.isEmpty()) {
+      return "\"\"";
+    } else {
+      boolean needsEscape = false;
+      for (int i = 0; i < s.length(); ++i) {
+        char c = s.charAt(i);
+        if (c == ' ' || c == '"') {
+          needsEscape = true;
+          break;
+        }
+      }
+      if (!needsEscape) {
+        return s;
+      }
+    }
+
+    StringBuilder result = new StringBuilder();
+    result.append('"');
+    int start = 0;
+    for (int i = 0; i < s.length(); ++i) {
+      char c = s.charAt(i);
+      if (c == '"' || c == '\\') {
+        // Copy the segment since the last special character.
+        if (start >= 0) {
+          result.append(s.substring(start, i));
+          start = -1;
+        }
+
+        // Handle the current special character.
+        if (c == '"') {
+          // This is a quote character. Escape it with a single backslash.
+          result.append("\\\"");
+        } else {
+          // This is a backslash (or the first one in a run of backslashes).
+          // Whether we escape it depends on whether the run ends with a quote.
+          int run_len = 1;
+          int j = i + 1;
+          while (j < s.length() && s.charAt(j) == '\\') {
+            run_len++;
+            j++;
+          }
+          if (j == s.length()) {
+            // The run of backslashes goes to the end.
+            // We have to escape every backslash with another backslash.
+            for (int k = 0; k < run_len * 2; ++k) {
+              result.append('\\');
+            }
+            break;
+          } else if (j < s.length() && s.charAt(j) == '"') {
+            // The run of backslashes is terminated by a quote.
+            // We have to escape every backslash with another backslash, and
+            // escape the quote with one backslash.
+            for (int k = 0; k < run_len * 2; ++k) {
+              result.append('\\');
+            }
+            result.append("\\\"");
+            i += run_len;  // 'i' is also increased in the loop iteration step
+          } else {
+            // No quote found. Each backslash counts for itself, they must not be
+            // escaped.
+            for (int k = 0; k < run_len; ++k) {
+              result.append('\\');
+            }
+            i += run_len - 1;  // 'i' is also increased in the loop iteration step
+          }
+        }
+      } else {
+        // This is not a special character. Start the segment if necessary.
+        if (start < 0) {
+          start = i;
+        }
+      }
+    }
+    // Save final segment after the last special character.
+    if (start != -1) {
+      result.append(s.substring(start));
+    }
+    result.append('"');
+    return result.toString();
+  }
 }

src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java

@@ -14,6 +14,10 @@
 
 package com.google.devtools.build.lib.windows;
 
+import com.google.common.base.Function;
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
@@ -44,7 +48,17 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
     // DO NOT quote argv0, createProcess will do it for us.
     String argv0 = processArgv0(argv.get(0));
     String argvRest =
-        argv.size() > 1 ? WindowsProcesses.quoteCommandLine(argv.subList(1, argv.size())) : "";
+        argv.size() > 1
+            ? Joiner.on(" ").join(
+                Iterables.transform(
+                    argv.subList(1, argv.size()),
+                    new Function<String, String>() {
+                      @Override
+                      public String apply(String s) {
+                        return ShellUtils.escapeCreateProcessArg(s);
+                      }
+                    }))
+            : "";
     byte[] env = convertEnvToNative(builder.getEnv());
 
     String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile());

src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java

@@ -215,60 +215,4 @@ public static int getpid() {
   }
 
   private static native int nativeGetpid();
-
-  // TODO(laszlocsomor): Audit this method and fix bugs. This method implements Bash quoting
-  // semantics but Windows quote semantics are different.
-  // More info: http://daviddeley.com/autohotkey/parameters/parameters.htm
-  public static String quoteCommandLine(List<String> argv) {
-    StringBuilder result = new StringBuilder();
-    for (int iArg = 0; iArg < argv.size(); iArg++) {
-      if (iArg != 0) {
-        result.append(" ");
-      }
-      String arg = argv.get(iArg);
-      if (arg.isEmpty()) {
-        result.append("\"\"");
-        continue;
-      }
-      boolean hasSpace = arg.contains(" ");
-      if (!arg.contains("\"") && !arg.contains("\\") && !hasSpace) {
-        // fast path. Just append the input string.
-        result.append(arg);
-      } else {
-        // We need to quote things if the argument contains a space.
-        if (hasSpace) {
-          result.append("\"");
-        }
-
-        for (int iChar = 0; iChar < arg.length(); iChar++) {
-          boolean lastChar = iChar == arg.length() - 1;
-          switch (arg.charAt(iChar)) {
-            case '"':
-              // Escape double quotes
-              result.append("\\\"");
-              break;
-            case '\\':
-              // Backslashes at the end of the string are quoted if we add quotes
-              if (lastChar) {
-                result.append(hasSpace ? "\\\\" : "\\");
-              } else {
-                // Backslashes everywhere else are quoted if they are followed by a
-                // quote or a backslash
-                result.append(
-                    arg.charAt(iChar + 1) == '"' || arg.charAt(iChar + 1) == '\\' ? "\\\\" : "\\");
-              }
-              break;
-            default:
-              result.append(arg.charAt(iChar));
-          }
-        }
-        // Add ending quotes if we added a quote at the beginning.
-        if (hasSpace) {
-          result.append("\"");
-        }
-      }
-    }
-
-    return result.toString();
-  }
 }

src/test/java/com/google/devtools/build/lib/shell/ShellUtilsTest.java

@@ -134,4 +134,58 @@ public void testTokenizeFailsOnUnterminatedQuotation() {
     assertTokenizeFails("-Dfoo='bar", "unterminated quotation");
     assertTokenizeFails("-Dfoo=\"b'ar", "unterminated quotation");
   }
+
+  private void assertCreateProcessEscaping(String arg, String expected) {
+    assertThat(ShellUtils.escapeCreateProcessArg(arg)).isEqualTo(expected);
+  }
+
+  @Test
+  public void testEscapeCreateProcessArg() {
+    assertCreateProcessEscaping("", "\"\"");
+    assertCreateProcessEscaping("with\"quote", "\"with\\\"quote\"");
+    assertCreateProcessEscaping("one\\backslash", "one\\backslash");
+    assertCreateProcessEscaping("one\\ backslash and \\space", "\"one\\ backslash and \\space\"");
+    assertCreateProcessEscaping("two\\\\backslashes", "two\\\\backslashes");
+    assertCreateProcessEscaping(
+        "two\\\\ backslashes \\\\and space", "\"two\\\\ backslashes \\\\and space\"");
+    assertCreateProcessEscaping("one\\\"x", "\"one\\\\\\\"x\"");
+    assertCreateProcessEscaping("two\\\\\"x", "\"two\\\\\\\\\\\"x\"");
+    assertCreateProcessEscaping("a \\ b", "\"a \\ b\"");
+    assertCreateProcessEscaping("a \\\" b", "\"a \\\\\\\" b\"");
+
+    assertCreateProcessEscaping("A", "A");
+    assertCreateProcessEscaping("\"a\"", "\"\\\"a\\\"\"");
+
+    assertCreateProcessEscaping("B C", "\"B C\"");
+    assertCreateProcessEscaping("\"b c\"", "\"\\\"b c\\\"\"");
+
+    assertCreateProcessEscaping("D\"E", "\"D\\\"E\"");
+    assertCreateProcessEscaping("\"d\"e\"", "\"\\\"d\\\"e\\\"\"");
+
+    assertCreateProcessEscaping("C:\\F G", "\"C:\\F G\"");
+    assertCreateProcessEscaping("\"C:\\f g\"", "\"\\\"C:\\f g\\\"\"");
+
+    assertCreateProcessEscaping("C:\\H\"I", "\"C:\\H\\\"I\"");
+    assertCreateProcessEscaping("\"C:\\h\"i\"", "\"\\\"C:\\h\\\"i\\\"\"");
+
+    assertCreateProcessEscaping("C:\\J\\\"K", "\"C:\\J\\\\\\\"K\"");
+    assertCreateProcessEscaping("\"C:\\j\\\"k\"", "\"\\\"C:\\j\\\\\\\"k\\\"\"");
+
+    assertCreateProcessEscaping("C:\\L M ", "\"C:\\L M \"");
+    assertCreateProcessEscaping("\"C:\\l m \"", "\"\\\"C:\\l m \\\"\"");
+
+    assertCreateProcessEscaping("C:\\N O\\", "\"C:\\N O\\\\\"");
+    assertCreateProcessEscaping("\"C:\\n o\\\"", "\"\\\"C:\\n o\\\\\\\"\"");
+
+    assertCreateProcessEscaping("C:\\P Q\\ ", "\"C:\\P Q\\ \"");
+    assertCreateProcessEscaping("\"C:\\p q\\ \"", "\"\\\"C:\\p q\\ \\\"\"");
+
+    assertCreateProcessEscaping("C:\\R\\S\\", "C:\\R\\S\\");
+    assertCreateProcessEscaping("C:\\R x\\S\\", "\"C:\\R x\\S\\\\\"");
+    assertCreateProcessEscaping("\"C:\\r\\s\\\"", "\"\\\"C:\\r\\s\\\\\\\"\"");
+    assertCreateProcessEscaping("\"C:\\r x\\s\\\"", "\"\\\"C:\\r x\\s\\\\\\\"\"");
+
+    assertCreateProcessEscaping("C:\\T U\\W\\", "\"C:\\T U\\W\\\\\"");
+    assertCreateProcessEscaping("\"C:\\t u\\w\\\"", "\"\\\"C:\\t u\\w\\\\\\\"\"");
+  }
 }

src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java

@@ -18,7 +18,8 @@
 import static java.nio.charset.StandardCharsets.UTF_16LE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.base.Joiner;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.windows.jni.WindowsProcesses;
@@ -65,16 +66,17 @@ public void terminateProcess() throws Exception {
     }
   }
 
-  private String mockArgs(String... args) {
-    List<String> argv = new ArrayList<>();
-
-    argv.add("-jar");
-    argv.add(mockSubprocess);
+  private static String escapeArgs(String... args) {
+    List<String> argv = new ArrayList<>(args.length);
     for (String arg : args) {
-      argv.add(arg);
+      argv.add(ShellUtils.escapeCreateProcessArg(arg));
     }
+    return Joiner.on(" ").join(argv);
+  }
 
-    return WindowsProcesses.quoteCommandLine(argv);
+  private String mockArgs(String... args) {
+    return "-jar " + ShellUtils.escapeCreateProcessArg(mockSubprocess).replace('/', '\\') + " " +
+               escapeArgs(args);
   }
 
   private void assertNoProcessError() throws Exception {
@@ -87,35 +89,32 @@ private void assertNoStreamError(long stream) throws Exception {
 
   @Test
   public void testDoesNotQuoteSimpleArg() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a"))).isEqualTo("x a");
+    assertThat(escapeArgs("x", "a")).isEqualTo("x a");
   }
 
   @Test
   public void testQuotesEmptyArg() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", ""))).isEqualTo("x \"\"");
+    assertThat(escapeArgs("x", "")).isEqualTo("x \"\"");
   }
 
   @Test
   public void testQuotesArgWithSpace() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a b")))
-        .isEqualTo("x \"a b\"");
+    assertThat(escapeArgs("x", "a b")).isEqualTo("x \"a b\"");
   }
 
   @Test
   public void testDoesNotQuoteArgWithBackslash() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\\b")))
-        .isEqualTo("x a\\b");
+    assertThat(escapeArgs("x", "a\\b")).isEqualTo("x a\\b");
   }
 
   @Test
   public void testDoesNotQuoteArgWithSingleQuote() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a'b"))).isEqualTo("x a'b");
+    assertThat(escapeArgs("x", "a'b")).isEqualTo("x a'b");
   }
 
   @Test
   public void testDoesNotQuoteArgWithDoubleQuote() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\"b")))
-        .isEqualTo("x a\\\"b");
+    assertThat(escapeArgs("x", "a\"b")).isEqualTo("x \"a\\\"b\"");
   }
 
   @Test