diff --git a/src/main/java/com/google/devtools/build/lib/shell/ShellUtils.java b/src/main/java/com/google/devtools/build/lib/shell/ShellUtils.java index 44006aa897345c..eb960bd8f98f3e 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/ShellUtils.java +++ b/src/main/java/com/google/devtools/build/lib/shell/ShellUtils.java @@ -142,4 +142,86 @@ public static void tokenize(List 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(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java index 3ed4a6de4d808e..98ead4909cdd92 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java +++ b/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() { + @Override + public String apply(String s) { + return ShellUtils.escapeCreateProcessArg(s); + } + })) + : ""; byte[] env = convertEnvToNative(builder.getEnv()); String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile()); diff --git a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java index 65b262088a1a9e..2c0f33684367be 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java +++ b/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 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(); - } } diff --git a/src/test/java/com/google/devtools/build/lib/shell/ShellUtilsTest.java b/src/test/java/com/google/devtools/build/lib/shell/ShellUtilsTest.java index 4ebe5386d2bab9..3355ebcd57e02b 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/ShellUtilsTest.java +++ b/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\\\\\\\"\""); + } } diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java index 91e4fe42ab413c..83c5272205eb06 100644 --- a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java +++ b/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 argv = new ArrayList<>(); - - argv.add("-jar"); - argv.add(mockSubprocess); + private static String escapeArgs(String... args) { + List 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