Fix backslash escaping in Windows parameter files - #31204
bd-innovation wants to merge 3 commits into
Conversation
Double backslashes before literal quotes and before the closing quote of a quoted argument. Quote empty arguments so they are not dropped. Add regression tests for backslashes, empty arguments, and argument boundaries.
| } | ||
|
|
||
| @Test | ||
| public void escapesBackslashesBeforeQuotes() { |
There was a problem hiding this comment.
Nit: Using @TestParameter on count would be more idiomatic.
There was a problem hiding this comment.
Done, thanks!
|
@bazel-io fork 9.3.0 |
meteorcloudy
left a comment
There was a problem hiding this comment.
Thanks, my agent had two nit comments, I think those makes sense, can you please address them?
| public static String escapeString(String argument) { | ||
| boolean needsSurroundingQuotes = containsWhitespace(argument); | ||
| boolean needsSurroundingQuotes = argument.isEmpty() || containsWhitespace(argument); | ||
| StringBuilder out = new StringBuilder(); |
There was a problem hiding this comment.
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:
| 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); |
There was a problem hiding this comment.
Done, thanks! Addressed both comments.
| * 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. |
There was a problem hiding this comment.
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.
Add a fast path for arguments without backslashes or quotes and pre-size the escape buffer. Update the WINDOWS parameter file documentation.
…#31204) (bazelbuild#31249) ### Description Fix backslash escaping in Windows parameter files and preserve empty arguments. For example, `C:\Program Files\SDK\` is currently written with a single backslash before the closing quote. That backslash escapes the quote, so the next argument can become part of the path. Double backslashes before literal quotes and before an added closing quote. Leave other backslashes unchanged. Fixes bazelbuild#31202 ### Validation - Regression tests fail with the old implementation and pass with the fix. - `bazel test //src/test/java/com/google/devtools/build/lib/util:UtilTests --test_filter=WindowsParamFileEscaperTest` passes (ten tests). - Verified the argument-boundary failure and fix with Clang's `--rsp-quoting=windows` mode on macOS. No Windows/MSVC end-to-end build was run. - Google Java Format and `git diff --check` pass. ### Build API Changes No. ### Checklist - [x] I have added tests for the new use cases. - [x] I have updated the method documentation. ### Release Notes RELNOTES: Windows parameter files now preserve empty arguments and correctly escape backslashes before quotes and at the end of quoted arguments. Closes bazelbuild#31204 COPYBARA_INTEGRATE_REVIEW=bazelbuild#31204 from bd-innovation:fix/windows-param-file-escaping 727767f PiperOrigin-RevId: 985220391 Change-Id: I57b594ca20fa5bf1f3b70dc895c978b4164c69f0 <!-- Thank you for contributing to Bazel! Please read the contribution guidelines: https://bazel.build/contribute --> ### Description <!-- Please provide a brief summary of the changes in this PR. --> ### Motivation <!-- Why is this change important? Does it fix a specific bug or add a new feature? If this PR fixes an existing issue, please link it here (e.g. "Fixes bazelbuild#1234"). --> ### Build API Changes <!-- Does this PR affect the Build API? (e.g. Starlark API, providers, command-line flags, native rules) If yes, please answer the following: 1. Has this been discussed in a design doc or issue? (Please link it) 2. Is the change backward compatible? 3. If it's a breaking change, what is the migration plan? --> No ### Checklist - [ ] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes <!-- If this is a new feature, please add 'RELNOTES[NEW]: <description>' here. If this is a breaking change, please add 'RELNOTES[INC]: <reason>' here. If this change should be mentioned in release notes, please add 'RELNOTES: <reason>' here. --> RELNOTES: None Commit bazelbuild@9056d39 Co-authored-by: David Jyo <220460779+bd-innovation@users.noreply.github.com>
Description
Fix backslash escaping in Windows parameter files and preserve empty arguments.
For example,
C:\Program Files\SDK\is currently written with a single backslash before the closing quote. That backslash escapes the quote, so the next argument can become part of the path.Double backslashes before literal quotes and before an added closing quote. Leave other backslashes unchanged.
Fixes #31202
Validation
bazel test //src/test/java/com/google/devtools/build/lib/util:UtilTests --test_filter=WindowsParamFileEscaperTestpasses (ten tests).--rsp-quoting=windowsmode on macOS. No Windows/MSVC end-to-end build was run.git diff --checkpass.Build API Changes
No.
Checklist
Release Notes
RELNOTES: Windows parameter files now preserve empty arguments and correctly escape backslashes before quotes and at the end of quoted arguments.