[Windows] Fix handling of \" in program name on cmd line.

[simon_tatham]

Summary

Bugzilla #47579: if you invoke clang on Windows via a pathname in
which a quoted section closes just after a backslash, e.g.

"C:\Program Files\Whatever\"clang.exe

then cmd.exe and CreateProcess will correctly find the binary, because
when they parse the program name at the start of the command line,
they don't regard the \ before the " as having any kind of escaping
effect. This is different from the behaviour of the Windows standard C
library when it parses the rest of the command line, which would
consider that \" not to close the quoted string.

But this confuses windows::GetCommandLineArguments, because the
Windows API function GetCommandLineW() will return a command line
containing that \" sequence, and cl::TokenizeWindowsCommandLine will
tokenize the whole string according to the C library's rules. So it
will misidentify where the program name stops and the arguments start.

To fix this, I've introduced a new variant function
cl::TokenizeWindowsCommandLineFull(), intended to be applied to the
string returned from GetCommandLineW(). It parses the first word of
the command line according to CreateProcess's rules, considering \ to
never be an escaping character; thereafter, it switches over to the C
library rules for the rest of the command line.

In the case where there are no further " on the command line, the
previous code was also managing to return no arguments at all, causing
an assertion failure later on in GetCommandLineArguments when it tried
to refer to Args[0]. That occurred because the tokenizer was throwing
away the final token if it contained a quoted string still unclosed at
the end of the command line, which also doesn't match the Windows CRT
handling. So I've fixed that too, and for extra safety, included a
final check before dereferencing Args[0], so that just in case we
still somehow get a zero-length argument vector, we'll at least
not crash.

[rnk]

I would really like to avoid adding another tokenization variant function. We tried to write this code exactly following the rules the CRT uses to produce the argv[] array supplied to main. How does the CRT startup code handle that? Does it have special handling for the first token to match CreateProcess here?

Is the idea that Windows paths are not allowed to contain quotes, so backslash is not allowed to have any escaping power? In other words, double quotes in the first token are treated similarly to bash single quote, they have no escape mechanism.

llvm/lib/Support/CommandLine.cpp

1038–1039

This seems like it could be a bug when the command line ends with an open quoted string. We should match the CRT logic. In the original example, the entire command line would become argv[0].

llvm/lib/Support/Windows/Process.inc

258

Seems reasonable.

[simon_tatham]

In D122914#3423312, @rnk wrote:

We tried to write this code exactly following the rules the CRT uses to produce the argv[] array supplied to main. How does the CRT startup code handle that? Does it have special handling for the first token to match CreateProcess here?

I haven't seen the source of the real Windows CRT startup code, I'm afraid. But observing it as a black box, yes, it really does seem to handle the first token specially. I compiled and ran this trivial test program with VS2019, which prints both the GetCommandLine raw string and the argv received by main:

#include <stdio.h>
#include <windows.h>
int main(int argc, char **argv) {
  for (int i = 0; i < argc; i++) printf("argv[%d] = [%s]\n", i, argv[i]);
  printf("cl = [%s]\n", GetCommandLineA());
}

Output with a test string consisting of the exact same syntax twice:

C:\Users\statham>".\"args ".\"args
argv[0] = [.\args]
argv[1] = [."args]
cl = [".\"args  ".\"args]

You can see that GetCommandLine has received the same string I passed to cmd.exe, without any pre-transformation by cmd or CreateProcess to make it more palatable to the CRT (which was another hypothesis I considered). But then the C library argv splitter has treated the two copies of the same string differently: in argv[0], the quotes have been discarded and the backslash kept, whereas in argv[1] the backslash has escaped the quote.

(And therefore you'd expect this command line to have ended in mid-quoted string – and running again with further text after the second copy of ".\"args, it's easy to see that that is indeed the case ...

C:\Users\statham>".\"args ".\"args one " two
argv[0] = [.\args]
argv[1] = [."args one ]
argv[2] = [two]
cl = [".\"args  ".\"args one " two]

... which supports your other comment that we ought not to be throwing away non-empty unclosed quoted arguments at the end of the command line.)

Is the idea that Windows paths are not allowed to contain quotes, so backslash is not allowed to have any escaping power? In other words, double quotes in the first token are treated similarly to bash single quote, they have no escape mechanism.

As I say, I don't know for 100% sure how the CRT really thinks. But what you say is exactly my interpretation (including the rationale about " not being a legal filename character).

I would really like to avoid adding another tokenization variant function.

I hear you. But the problem is that the command-line tokenizer in llvm/Support is used both for full command lines as received from GetCommandLine and for snippets in things like response files, which only contain arguments intended to come after the command name. And surely one of those needs this special-casing of the first token, and the other one does not, if they're to be parsed correctly. So I don't see how to avoid having two functions, or one function with a boolean flag, or some way to trigger two different kinds of parsing for two contexts.

llvm/lib/Support/CommandLine.cpp

1038–1039

I thought the same, and I considered fixing it as part of the same commit – I would have expected that we'd add the current token if it had accumulated any text, whether the quotes had been closed or not. But the git history tells me that D78346 changed it from that to this, so I didn't want to unilaterally revert a previous deliberate change.

However, now I look more closely at that previous change, it looks as if it's not intentionally discarding any actual text – it's _preventing_ the discarding of an explicit empty-string token. So perhaps I can fix that after all without reverting the intended change. I think we ought to be testing State != INIT.

[simon_tatham]

Updated to fix the handling of a trailing unclosed quoted string, as suggested. It turns out (checking with that same 6-line test program) that even an empty unclosed quoted string still turns into an argv word, contrary to one of the existing unit tests.

[simon_tatham]

Fixed a CI failure (unreachable in tokenizeWindowsCommandLineImpl was reached because I forgot to correctly handle an unquoted initial backslash in CommandName mode), and added an extra unit test to ensure the triggering case stays working.

[simon_tatham]

Gentle ping: @rnk, are you satisfied with my changes and explanations from last week?

[rnk]

Thanks for running the experiments, I think I'm convinced we need to do this. @hans, can you review the implementation? I removed Adrian and Rui, who are no longer regular contributors.

[hans]

Looks good to me.

llvm/lib/Support/CommandLine.cpp

1038–1039

Perhaps this should be done in a separate commit?

[simon_tatham]

llvm/lib/Support/CommandLine.cpp

1038–1039

OK, I split it up at the last minute, and committed rG1be024ee450f2d3cb07086f6141d50f291c1910b for the problem with a trailing unclosed quoted string, and rG32814df442690d4673759296d850804773a7ea5b for the problem with \" in the command name.

Thanks for the review!

llvm/include/llvm/Support/CommandLine.h

 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
 /// \param [in] MarkEOLs true if tokenizing a response file and you want end of
 /// lines and end of the response file to be marked with a nullptr string.
 /// \param [out] NewArgv All parsed strings are appended to NewArgv.
 void TokenizeGNUCommandLine(StringRef Source, StringSaver &Saver,
                             SmallVectorImpl<const char *> &NewArgv,
                             bool MarkEOLs = false);
 
-/// Tokenizes a Windows command line which may contain quotes and escaped
-/// quotes.
+/// Tokenizes a string of Windows command line arguments, which may contain
+/// quotes and escaped quotes.
 ///
 /// See MSDN docs for CommandLineToArgvW for information on the quoting rules.
 /// http://msdn.microsoft.com/en-us/library/windows/desktop/17w5ykft(v=vs.85).aspx
 ///
+/// For handling a full Windows command line including the executable name at
+/// the start, see TokenizeWindowsCommandLineFull below.
+///
 /// \param [in] Source The string to be split on whitespace with quotes.
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
 /// \param [in] MarkEOLs true if tokenizing a response file and you want end of
 /// lines and end of the response file to be marked with a nullptr string.
 /// \param [out] NewArgv All parsed strings are appended to NewArgv.
 void TokenizeWindowsCommandLine(StringRef Source, StringSaver &Saver,
                                 SmallVectorImpl<const char *> &NewArgv,
                                 bool MarkEOLs = false);
 
 /// Tokenizes a Windows command line while attempting to avoid copies. If no
 /// quoting or escaping was used, this produces substrings of the original
 /// string. If a token requires unquoting, it will be allocated with the
 /// StringSaver.
 void TokenizeWindowsCommandLineNoCopy(StringRef Source, StringSaver &Saver,
                                       SmallVectorImpl<StringRef> &NewArgv);
 
+/// Tokenizes a Windows full command line, including command name at the start.
+///
+/// This uses the same syntax rules as TokenizeWindowsCommandLine for all but
+/// the first token. But the first token is expected to be parsed as the
+/// executable file name in the way CreateProcess would do it, rather than the
+/// way the C library startup code would do it: CreateProcess does not consider
+/// that \ is ever an escape character (because " is not a valid filename char,
+/// hence there's never a need to escape it to be used literally).
+///
+/// Parameters are the same as for TokenizeWindowsCommandLine. In particular,
+/// if you set MarkEOLs = true, then the first word of every line will be
+/// parsed using the special rules for command names, making this function
+/// suitable for parsing a file full of commands to execute.
+void TokenizeWindowsCommandLineFull(StringRef Source, StringSaver &Saver,
+                                    SmallVectorImpl<const char *> &NewArgv,
+                                    bool MarkEOLs = false);
+
 /// String tokenization function type.  Should be compatible with either
 /// Windows or Unix command line tokenizers.
 using TokenizerCallback = void (*)(StringRef Source, StringSaver &Saver,
                                    SmallVectorImpl<const char *> &NewArgv,
                                    bool MarkEOLs);
 
 /// Tokenizes content of configuration file.
 ///

llvm/lib/Support/CommandLine.cpp

if (BackslashCount % 2 == 0)
       return I - 1;
     Token.push_back('"');
     return I;
   }
   Token.append(BackslashCount, '\\');
   return I - 1;
 }
 
-// Windows treats whitespace, double quotes, and backslashes specially.
+// Windows treats whitespace, double quotes, and backslashes specially, except
+// when parsing the first token of a full command line, in which case
+// backslashes are not special.
 static bool isWindowsSpecialChar(char C) {
   return isWhitespaceOrNull(C) || C == '\\' || C == '\"';
 }
+static bool isWindowsSpecialCharInCommandName(char C) {
+  return isWhitespaceOrNull(C) || C == '\"';
+}
 
 // Windows tokenization implementation. The implementation is designed to be
 // inlined and specialized for the two user entry points.
-static inline void
-tokenizeWindowsCommandLineImpl(StringRef Src, StringSaver &Saver,
-                               function_ref<void(StringRef)> AddToken,
-                               bool AlwaysCopy, function_ref<void()> MarkEOL) {
+static inline void tokenizeWindowsCommandLineImpl(
+    StringRef Src, StringSaver &Saver, function_ref<void(StringRef)> AddToken,
+    bool AlwaysCopy, function_ref<void()> MarkEOL, bool InitialCommandName) {
   SmallString<128> Token;
 
+  // Sometimes, this function will be handling a full command line including an
+  // executable pathname at the start. In that situation, the initial pathname
+  // needs different handling from the following arguments, because when
+  // CreateProcess or cmd.exe scans the pathname, it doesn't treat \ as
+  // escaping the quote character, whereas when libc scans the rest of the
+  // command line, it does.
+  bool CommandName = InitialCommandName;
+
   // Try to do as much work inside the state machine as possible.
   enum { INIT, UNQUOTED, QUOTED } State = INIT;
+
   for (size_t I = 0, E = Src.size(); I < E; ++I) {
     switch (State) {
     case INIT: {
       assert(Token.empty() && "token should be empty in initial state");
       // Eat whitespace before a token.
       while (I < E && isWhitespaceOrNull(Src[I])) {
         if (Src[I] == '\n')
           MarkEOL();
         ++I;
       }
       // Stop if this was trailing whitespace.
       if (I >= E)
         break;
       size_t Start = I;
+      if (CommandName) {
+        while (I < E && !isWindowsSpecialCharInCommandName(Src[I]))
+          ++I;
+      } else {
         while (I < E && !isWindowsSpecialChar(Src[I]))
           ++I;
+      }
       StringRef NormalChars = Src.slice(Start, I);
       if (I >= E || isWhitespaceOrNull(Src[I])) {
         // No special characters: slice out the substring and start the next
         // token. Copy the string if the caller asks us to.
         AddToken(AlwaysCopy ? Saver.save(NormalChars) : NormalChars);
-        if (I < E && Src[I] == '\n')
+        if (I < E && Src[I] == '\n') {
           MarkEOL();
+          CommandName = InitialCommandName;
+        } else {
+          CommandName = false;
+        }
       } else if (Src[I] == '\"') {
         Token += NormalChars;
         State = QUOTED;
       } else if (Src[I] == '\\') {
+        assert(!CommandName && "or else we'd have treated it as a normal char");
         Token += NormalChars;
         I = parseBackslash(Src, I, Token);
         State = UNQUOTED;
       } else {
         llvm_unreachable("unexpected special character");
       }
       break;
     }
 
     case UNQUOTED:
       if (isWhitespaceOrNull(Src[I])) {
         // Whitespace means the end of the token. If we are in this state, the
         // token must have contained a special character, so we must copy the
         // token.
         AddToken(Saver.save(Token.str()));
         Token.clear();
-        if (Src[I] == '\n')
+        if (Src[I] == '\n') {
+          CommandName = InitialCommandName;
           MarkEOL();
+        } else {
+          CommandName = false;
+        }
         State = INIT;
       } else if (Src[I] == '\"') {
         State = QUOTED;
-      } else if (Src[I] == '\\') {
+      } else if (Src[I] == '\\' && !CommandName) {
         I = parseBackslash(Src, I, Token);
       } else {
         Token.push_back(Src[I]);
       }
       break;
 
     case QUOTED:
       if (Src[I] == '\"') {
         if (I < (E - 1) && Src[I + 1] == '"') {
           // Consecutive double-quotes inside a quoted string implies one
           // double-quote.
           Token.push_back('"');
           ++I;
         } else {
           // Otherwise, end the quoted portion and return to the unquoted state.
           State = UNQUOTED;
         }
-      } else if (Src[I] == '\\') {
+      } else if (Src[I] == '\\' && !CommandName) {
         I = parseBackslash(Src, I, Token);
       } else {
         Token.push_back(Src[I]);
       }
       break;
     }
   }
 
   if (State != INIT)
     AddToken(Saver.save(Token.str()));
 }
 
 void cl::TokenizeWindowsCommandLine(StringRef Src, StringSaver &Saver,
                                     SmallVectorImpl<const char *> &NewArgv,
                                     bool MarkEOLs) {
   auto AddToken = [&](StringRef Tok) { NewArgv.push_back(Tok.data()); };
   auto OnEOL = [&]() {
     if (MarkEOLs)
       NewArgv.push_back(nullptr);
   };
   tokenizeWindowsCommandLineImpl(Src, Saver, AddToken,
-                                 /*AlwaysCopy=*/true, OnEOL);
+                                 /*AlwaysCopy=*/true, OnEOL, false);
 }
 
 void cl::TokenizeWindowsCommandLineNoCopy(StringRef Src, StringSaver &Saver,
                                           SmallVectorImpl<StringRef> &NewArgv) {
   auto AddToken = [&](StringRef Tok) { NewArgv.push_back(Tok); };
   auto OnEOL = []() {};
   tokenizeWindowsCommandLineImpl(Src, Saver, AddToken, /*AlwaysCopy=*/false,
-                                 OnEOL);
+                                 OnEOL, false);
+}
+
+void cl::TokenizeWindowsCommandLineFull(StringRef Src, StringSaver &Saver,
+                                        SmallVectorImpl<const char *> &NewArgv,
+                                        bool MarkEOLs) {
+  auto AddToken = [&](StringRef Tok) { NewArgv.push_back(Tok.data()); };
+  auto OnEOL = [&]() {
+    if (MarkEOLs)
+      NewArgv.push_back(nullptr);
+  };
+  tokenizeWindowsCommandLineImpl(Src, Saver, AddToken,
+                                 /*AlwaysCopy=*/true, OnEOL, true);
 }
 
 void cl::tokenizeConfigFile(StringRef Source, StringSaver &Saver,
                             SmallVectorImpl<const char *> &NewArgv,
                             bool MarkEOLs) {
   for (const char *Cur = Source.begin(); Cur != Source.end();) {
     SmallString<128> Line;
     // Check for comment line.

llvm/lib/Support/Windows/Process.inc

windows::GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
   std::error_code EC;
   SmallString<MAX_PATH> Cmd;
   EC = windows::UTF16ToUTF8(CmdW, wcslen(CmdW), Cmd);
   if (EC)
     return EC;
 
   SmallVector<const char *, 20> TmpArgs;
   StringSaver Saver(Alloc);
-  cl::TokenizeWindowsCommandLine(Cmd, Saver, TmpArgs, /*MarkEOLs=*/false);
+  cl::TokenizeWindowsCommandLineFull(Cmd, Saver, TmpArgs, /*MarkEOLs=*/false);
 
   for (const char *Arg : TmpArgs) {
     EC = WildcardExpand(Arg, Args, Saver);
     if (EC)
       return EC;
   }
 
   if (Args.size() == 0)
     return std::make_error_code(std::errc::invalid_argument);
 
   SmallVector<char, MAX_PATH> Arg0(Args[0], Args[0] + strlen(Args[0]));
   SmallVector<char, MAX_PATH> Filename;
   sys::path::remove_filename(Arg0);
   EC = GetExecutableName(Filename);
   if (EC)
     return EC;

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, TokenizeWindowsCommandLineQuotedLastArgument) {
   const char Input2[] = R"(a b c d ")";
   const char *const Output2[] = {"a", "b", "c", "d", ""};
   testCommandLineTokenizer(cl::TokenizeWindowsCommandLine, Input2, Output2);
   const char Input3[] = R"(a b c d "text)";
   const char *const Output3[] = {"a", "b", "c", "d", "text"};
   testCommandLineTokenizer(cl::TokenizeWindowsCommandLine, Input3, Output3);
 }
 
+TEST(CommandLineTest, TokenizeWindowsCommandLineExeName) {
+  const char Input1[] =
+      R"("C:\Program Files\Whatever\"clang.exe z.c -DY=\"x\")";
+  const char *const Output1[] = {"C:\\Program Files\\Whatever\\clang.exe",
+                                 "z.c", "-DY=\"x\""};
+  testCommandLineTokenizer(cl::TokenizeWindowsCommandLineFull, Input1, Output1);
+
+  const char Input2[] = "\"a\\\"b c\\\"d\n\"e\\\"f g\\\"h\n";
+  const char *const Output2[] = {"a\\b", "c\"d", nullptr,
+                                 "e\\f", "g\"h", nullptr};
+  testCommandLineTokenizer(cl::TokenizeWindowsCommandLineFull, Input2, Output2,
+                           /*MarkEOLs=*/true);
+
+  const char Input3[] = R"(\\server\share\subdir\clang.exe)";
+  const char *const Output3[] = {"\\\\server\\share\\subdir\\clang.exe"};
+  testCommandLineTokenizer(cl::TokenizeWindowsCommandLineFull, Input3, Output3);
+}
+
 TEST(CommandLineTest, TokenizeAndMarkEOLs) {
   // Clang uses EOL marking in response files to support options that consume
   // the rest of the arguments on the current line, but do not consume arguments
   // from subsequent lines. For example, given these rsp files contents:
   // /c /Zi /O2
   // /Oy- /link /debug /opt:ref
   // /Zc:ThreadsafeStatics-
   //