[sanitizer][win] Change cmdline check to allow double backslashs

[alvinhochun]

Summary

When llvm-symbolizer.exe is on the PATH in an entry containing two
consecutive backslashes, sanitizers will try to launch llvm-symbolizer
with its absolute path containing these consecutive backslashes. This
fails a sanity check in sanitizer_symbolizer_win.cpp.

According to the documentation of CommandLineToArgvW [1] and a MS blog
post [2], backslashes in general, regardless of how many of them in a
row, do not have any special effect, unless when immediately followed by
a double quote.

There already exists a check that fails when the command line arguments
contains double quote, therefore the check for double backslashes can
simply be removed.

[1]: https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw
[2]: https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

[mstorsjo]

Maybe adjust the commit message a little,

multiple consecutive backslashes does not have any special effect, unless they are immediately followed by a double quote.

I would say backslashes in general, regardless of whether they're single or multiple, don't have any special effect unless followed by a double quote.

Other than that, I think this change is fine (I presume you tested running in such a setup); I added a couple others as reviewers as well, so maybe wait for a day or two if they want to chime in on the matter too.

[tejohnson]

I'm not a windows expert, but looking at the references this seems ok. The check was introduced with D11791. Looks like there is one windows test of the symbolizer path, added with that patch (compiler-rt/test/asan/TestCases/Windows/report_after_syminitialize.cpp). Can you add a test that it accepts a double backslash path with this change (like that existing path, presumably we can error that it wasn't found, but it shouldn't get the removed check's error anymore)? Perhaps that existing test can just be tweaked to add the double backslash onto the provided symbolizer path.

[alvinhochun]

Reworded commit a bit and changed the report_after_syminitialize test to test with double backslashes in the symbolizer path.

I did test this patch manually, but I don't have an environment for running the lit test so I may have to ask @mstorsjo to try that. Sorry to bother you :)

[mstorsjo]

In D146621#4213136, @alvinhochun wrote:

I did test this patch manually, but I don't have an environment for running the lit test so I may have to ask @mstorsjo to try that. Sorry to bother you :)

I believe the tests still pass after this, so this should be good to go - thanks!

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

bool SymbolizerProcess::StartSymbolizerSubprocess() {
   GetArgV(path_, argv);
   InternalScopedString command_line;
   for (int i = 0; argv[i]; i++) {
     const char *arg = argv[i];
     int arglen = internal_strlen(arg);
     // Check that tool command lines are simple and that complete escaping is
     // unnecessary.
     CHECK(!internal_strchr(arg, '"') && "quotes in args unsupported");
-    CHECK(!internal_strstr(arg, "\\\\") &&
-          "double backslashes in args unsupported");
     CHECK(arglen > 0 && arg[arglen - 1] != '\\' &&
           "args ending in backslash and empty args unsupported");
     command_line.append("\"%s\" ", arg);
   }
   VReport(3, "Launching symbolizer command: %s\n", command_line.data());
 
   // Launch llvm-symbolizer with stdin and stdout redirected.
   STARTUPINFOA si;

compiler-rt/test/asan/TestCases/Windows/report_after_syminitialize.cpp

 // RUN: %clangxx_asan -O0 %s -o %t
-// RUN: %env_asan_opts=external_symbolizer_path=asdf not %run %t 2>&1 | FileCheck %s
+// RUN: %env_asan_opts=external_symbolizer_path=non-existent\\\\asdf not %run %t 2>&1 | FileCheck %s
 
 #include <windows.h>
 #include <dbghelp.h>
 
 #pragma comment(lib, "dbghelp")
 
 int main() {
   // Make sure the RTL recovers from "no options enabled" dbghelp setup.