Skip to content

Fix backslash escaping in Windows parameter files #31204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bd-innovation wants to merge 3 commits into from
+67 −9
Closed

Fix backslash escaping in Windows parameter files #31204

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36daa50
Fix backslash escaping in Windows parameter files
bd-innovation Sep 18, 2026
ecfc1e0
Use TestParameter for Windows parameter escaping tests
bd-innovation Sep 18, 2026
727767f
Avoid unnecessary copying in Windows parameter escaping
bd-innovation Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/main/java/com/google/devtools/build/lib/util/WindowsParamFileEscaper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,41 @@ private WindowsParamFileEscaper() {}

/**
* Escapes the @argument to be suitable for lld-link. Existing double-quotes are escaped, and
* arguments that contain whitespace are surrounded in unescaped double-quotes.
* empty arguments and arguments that contain whitespace are surrounded in unescaped
* double-quotes. Backslashes immediately before a double-quote are doubled.
Comment on lines 25 to +27

@meteorcloudy meteorcloudy Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this javadoc. Could you also update the other copy of it that documents the same format for users? ParameterFileType.WINDOWS in ParameterFile.java still describes the old behavior:

/**
 * A parameter file where each parameter is correctly quoted for windows use. Double-quotes are
 * escaped, and each parameter that contains whitespace is surrounded in double-quotes.
 */
WINDOWS,

Something like:

A parameter file where each parameter is correctly quoted for Windows use. Double-quotes are escaped, backslashes immediately preceding a double-quote are doubled, and empty parameters as well as parameters containing whitespace are surrounded in double-quotes.

*
* @see <a
* href="https://github.com/llvm/llvm-project/blob/4bc3b3501ff994fb3504ed2b973342821a9c8cea/llvm/lib/Support/CommandLine.cpp#L916">LLVM
* Parser Implementation</a>
*/
public static String escapeString(String argument) {
boolean needsSurroundingQuotes = containsWhitespace(argument);
boolean needsSurroundingQuotes = argument.isEmpty() || containsWhitespace(argument);
StringBuilder out = new StringBuilder();

@meteorcloudy meteorcloudy Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough fix and tests — I verified that the new output round-trips correctly through LLVM's Windows tokenizer (clang --rsp-quoting=windows) for backslash runs of 1–4, UNC paths, embedded quotes and trailing backslashes.

One perf consideration: escapeString is on a fairly hot path — ParameterFile.writeParameterFile calls it once per argument, and link/archive param files can contain 100k+ arguments. The previous implementation used String.replace, which is intrinsified and allocation-free when there is nothing to replace; the new one walks every character and grows an unsized StringBuilder (default capacity 16) for every argument, including the overwhelming majority that contain no \ or ".

Could we keep a fast path for those and pre-size the builder otherwise? Semantics are unchanged, since with no backslashes and no quotes the loop is a verbatim copy and the trailing-backslash handling is a no-op:

Suggested change
public static String escapeString(String argument) {
boolean needsSurroundingQuotes = containsWhitespace(argument);
boolean needsSurroundingQuotes = argument.isEmpty() || containsWhitespace(argument);
StringBuilder out = new StringBuilder();
public static String escapeString(String argument) {
if (!argument.isEmpty() && argument.indexOf('\\') < 0 && argument.indexOf('"') < 0) {
return containsWhitespace(argument) ? "\"" + argument + "\"" : argument;
}
boolean needsSurroundingQuotes = argument.isEmpty() || containsWhitespace(argument);
StringBuilder out = new StringBuilder(argument.length() + 2);

@bd-innovation bd-innovation Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks! Addressed both comments.

if (needsSurroundingQuotes) {
out.append("\"");
}
out.append(argument.replace("\"", "\\\""));
int backslashes = 0;
for (int i = 0; i < argument.length(); i++) {
char c = argument.charAt(i);
if (c == '\\') {
backslashes++;
continue;
}
if (c == '"') {
// Each literal backslash needs a pair, followed by one to escape the quote.
out.append("\\".repeat(2 * backslashes + 1));
} else {
out.append("\\".repeat(backslashes));
}
out.append(c);
backslashes = 0;
}
if (needsSurroundingQuotes) {
// Keep trailing backslashes from escaping the closing quote.
out.append("\\".repeat(2 * backslashes));
out.append("\"");
} else {
out.append("\\".repeat(backslashes));
}
return out.toString();
}
Expand Down
40 changes: 37 additions & 3 deletions src/test/java/com/google/devtools/build/lib/util/WindowsParamFileEscaperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,20 @@
import static com.google.devtools.build.lib.util.WindowsParamFileEscaper.escapeString;

import com.google.common.collect.ImmutableSet;
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.util.Arrays;
import java.util.Set;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Tests for {@link WindowsParamFileEscaper}. */
@RunWith(JUnit4.class)
@RunWith(TestParameterInjector.class)
public class WindowsParamFileEscaperTest {

@Test
public void testEscapeString() throws Exception {
assertThat(escapeString("")).isEmpty();
assertThat(escapeString("")).isEqualTo("\"\"");
assertThat(escapeString("foo")).isEqualTo("foo");
assertThat(escapeString("'foo'")).isEqualTo("'foo'");
assertThat(escapeString("\"foo\"")).isEqualTo("\\\"foo\\\"");
Expand All @@ -44,6 +45,39 @@ public void testEscapeString() throws Exception {
assertThat(escapeString("${filename%.c}.o")).isEqualTo("${filename%.c}.o");
}

@Test
public void escapesBackslashesBeforeQuotes(@TestParameter({"1", "2", "3"}) int count) {
String backslashes = "\\".repeat(count);
String escapedBackslashes = "\\".repeat(2 * count + 1);
assertThat(escapeString("foo" + backslashes + "\"bar"))
.isEqualTo("foo" + escapedBackslashes + "\"bar");
assertThat(escapeString("foo " + backslashes + "\"bar"))
.isEqualTo("\"foo " + escapedBackslashes + "\"bar\"");
}

@Test
public void escapesTrailingBackslashesInQuotedArguments(
@TestParameter({"1", "2", "3"}) int count) {
assertThat(escapeString("C:\\Program Files\\SDK" + "\\".repeat(count)))
.isEqualTo("\"C:\\Program Files\\SDK" + "\\".repeat(2 * count) + "\"");
}

@Test
public void preservesBackslashesWithoutFollowingQuotes() {
assertThat(escapeString("C:\\SDK\\")).isEqualTo("C:\\SDK\\");
assertThat(escapeString("\\\\server\\share\\")).isEqualTo("\\\\server\\share\\");
assertThat(escapeString("C:\\Program Files\\SDK")).isEqualTo("\"C:\\Program Files\\SDK\"");
}

@Test
public void escapeAllPreservesArgumentBoundaries() {
assertThat(
WindowsParamFileEscaper.escapeAll(
Arrays.asList("", "C:\\Program Files\\SDK\\", "next", "")))
.containsExactly("\"\"", "\"C:\\Program Files\\SDK\\\\\"", "next", "\"\"")
.inOrder();
}

@Test
public void testEscapeAll() throws Exception {
Set<String> escaped =
Expand Down