[Windows] Fix limit on command line size

[sepavloff]

Summary

Documentation on CreateProcessW states that maximal size of command line
is 32767 characters including ternimation null character. In the
function llvm::sys::commandLineFitsWithinSystemLimits this limit was set
to 32768. As a result if command line was exactly 32768 characters long,
a response file was not created and CreateProcessW was called with
too long command line.

[amccarth]

I'm jumping in since rnk is on leave and (I believe) zturner is less focused on these issues than he used to be.

Thanks for tracking down the cause of this bug.

I have some concerns:

• We're trying to cut it right up to the hard limit. That seems an unnecessary risk, as the off-by-one bug illustrates. Is there anything wrong with just rounding down to 32,000 and leaving ourselves some wiggle room? Will someone be upset that we used a response file for a _very_ long command that technically would have been a viable command line?

• We're constructing a temporary copy of the command line to test if it's short enough and then constructing the actual command line elsewhere, which leaves us open to divergence between when we think the command line will be and what it actually ends up being. I'd rather measure the actual command line, especially if we're going to run right up to the limit.

• We're checking to make sure that the number of UTF-8 code units is below the limit, but the limit is actually in terms of WCHARs. Fortunately, I think that discrepancy works in our favor, since the number of WCHARs will always be smaller than the number of UTF-8 code units. But it underscores the points I'm making above: the precise limit probably isn't an issue and we're measuring a proxy command line rather than the actual command line.

Please consider these suggestions:

• Round down to 32,000 to leave us wiggle room.

• Have flattenWindowsCommandLine return a wstring rather than a string. This will reduce the chance of the proxy string we measure differing from the actual command string we're issuing, and it's already a Windows-specific function. This will require a change in Execute (in the same source file), which is currently doing the conversion from UTF-8 to UTF-16.

• Add a short command to the test for commandLineFitsWithinSystemLimits. Right now, the test only checks a humongous command line. (The test is in llvm\unittests\Support\CommandLineTest.cpp.)

[sepavloff]

Addressed reviewer's notes

[sepavloff]

Thank you for your detailed feedback!

Round down to 32,000 to leave us wiggle room.

As there is no requirement to use response files as rarely as possible, the choice of the limit is an implementation detail. Having some wiggle room is a good idea in this case.

Have flattenWindowsCommandLine return a wstring rather than a string. This will reduce the chance of the proxy string we measure differing from the actual command string we're issuing, and it's already a Windows-specific function.

Agree, this is more robust solution.

Add a short command to the test for commandLineFitsWithinSystemLimits.

Now the patch is more than a change of a constant, so a test is necessary.

[sepavloff]

Fixed typo

[amccarth]

Thanks! I know it was more work, but I think unifying the command line generation to work the same in both cases will avoid future surprises.

[sepavloff]

Fixed unit test

[max-kudr]

This commit is now failing LLDB Windows buildbot builds http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/17702. Please fix or revert.

E:\build_slave\lldb-x64-windows-ninja\llvm-project\lldb\source\Host\windows\ProcessLauncherWindows.cpp(53): error C2679: binary '=': no operator found which takes a right-hand operand of type 'llvm::ErrorOr<std::wstring>' (or there is no acceptable conversion)

[amccarth]

In D83772#2164685, @max-kudr wrote:

This commit is now failing LLDB Windows buildbot builds http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/17702. Please fix or revert.

E:\build_slave\lldb-x64-windows-ninja\llvm-project\lldb\source\Host\windows\ProcessLauncherWindows.cpp(53): error C2679: binary '=': no operator found which takes a right-hand operand of type 'llvm::ErrorOr<std::wstring>' (or there is no acceptable conversion)

I'll fix that.

[max-kudr]

Thank you! @sepavloff reverted it in revision ac0edc55887b6961ad90fd51f349c9587b1a8a7a

[sepavloff]

Added changes for LLDB

[sepavloff]

@k8stone @max-kudr Could you please review the changes for LLDB?

[max-kudr]

LGTM

llvm/include/llvm/Support/Program.h

#endif
   /// Print a command argument, and optionally quote it.
   void printArg(llvm::raw_ostream &OS, StringRef Arg, bool Quote);
 
 #if defined(_WIN32)
   /// Given a list of command line arguments, quote and escape them as necessary
   /// to build a single flat command line appropriate for calling CreateProcess
   /// on
   /// Windows.
-  std::string flattenWindowsCommandLine(ArrayRef<StringRef> Args);
+  ErrorOr<std::wstring> flattenWindowsCommandLine(ArrayRef<StringRef> Args);
 #endif
   }
 }
 
 #endif

llvm/lib/Support/Windows/Program.inc

static bool Execute(ProcessInfo &PI, StringRef Program,
   // ".exe" ourselves.
   SmallString<64> ProgramStorage;
   if (!sys::fs::exists(Program))
     Program = Twine(Program + ".exe").toStringRef(ProgramStorage);
 
   // Windows wants a command line, not an array of args, to pass to the new
   // process.  We have to concatenate them all, while quoting the args that
   // have embedded spaces (or are empty).
-  std::string Command = flattenWindowsCommandLine(Args);
+  auto Result = flattenWindowsCommandLine(Args);
+  if (std::error_code ec = Result.getError()) {
+    SetLastError(ec.value());
+    MakeErrMsg(ErrMsg,
+               std::string("Unable to convert command-line to UTF-16"));
+    return false;
+  }
+  std::wstring Command = *Result;
 
   // The pointer to the environment block for the new process.
   std::vector<wchar_t> EnvBlock;
 
   if (Env) {
     // An environment block consists of a null-terminated block of
     // null-terminated strings. Convert the array of environment variables to
     // an environment block by concatenating them.
static bool Execute(ProcessInfo &PI, StringRef Program,
   SmallVector<wchar_t, MAX_PATH> ProgramUtf16;
   if (std::error_code ec = sys::windows::widenPath(Program, ProgramUtf16)) {
     SetLastError(ec.value());
     MakeErrMsg(ErrMsg,
                std::string("Unable to convert application name to UTF-16"));
     return false;
   }
 
-  SmallVector<wchar_t, MAX_PATH> CommandUtf16;
-  if (std::error_code ec = windows::UTF8ToUTF16(Command, CommandUtf16)) {
-    SetLastError(ec.value());
-    MakeErrMsg(ErrMsg,
-               std::string("Unable to convert command-line to UTF-16"));
-    return false;
-  }
-
+  std::vector<wchar_t> CommandUtf16(Command.size() + 1, 0);
+  std::copy(Command.begin(), Command.end(), CommandUtf16.begin());
   BOOL rc = CreateProcessW(ProgramUtf16.data(), CommandUtf16.data(), 0, 0,
                            TRUE, CREATE_UNICODE_ENVIRONMENT,
                            EnvBlock.empty() ? 0 : EnvBlock.data(), 0, &si,
                            &pi);
   DWORD err = GetLastError();
 
   // Regardless of whether the process got created or not, we are done with
   // the handles we created for it to inherit.
while (!Arg.empty()) {
     Arg = Arg.drop_front(FirstNonBackslash + 1);
   }
 
   Result.push_back('"');
   return Result;
 }
 
 namespace llvm {
-std::string sys::flattenWindowsCommandLine(ArrayRef<StringRef> Args) {
+ErrorOr<std::wstring> sys::flattenWindowsCommandLine(ArrayRef<StringRef> Args) {
   std::string Command;
   for (StringRef Arg : Args) {
     if (argNeedsQuotes(Arg))
       Command += quoteSingleArg(Arg);
     else
       Command += Arg;
 
     Command.push_back(' ');
   }
 
-  return Command;
+  SmallVector<wchar_t, MAX_PATH> CommandUtf16;
+  if (std::error_code ec = windows::UTF8ToUTF16(Command, CommandUtf16))
+    return ec;
+
+  return std::wstring(CommandUtf16.begin(), CommandUtf16.end());
 }
 
 ProcessInfo sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
                       bool WaitUntilChildTerminates, std::string *ErrMsg,
                       Optional<ProcessStatistics> *ProcStat) {
   assert(PI.Pid && "invalid pid to wait on, process not started?");
   assert((PI.Process && PI.Process != INVALID_HANDLE_VALUE) &&
          "invalid process handle to wait on, process not started?");
llvm::sys::writeFileWithEncoding(StringRef FileName, StringRef Contents,
   if (OS.has_error())
     return make_error_code(errc::io_error);
 
   return EC;
 }
 
 bool llvm::sys::commandLineFitsWithinSystemLimits(StringRef Program,
                                                   ArrayRef<StringRef> Args) {
-  // The documented max length of the command line passed to CreateProcess.
-  static const size_t MaxCommandStringLength = 32768;
+  // The documentation on CreateProcessW states that the size of the argument
+  // lpCommandLine must not be greater than 32767 characters, including the
+  // Unicode terminating null character. We use smaller value to reduce risk
+  // of getting invalid command line due to unaccounted factors.
+  static const size_t MaxCommandStringLength = 32000;
   SmallVector<StringRef, 8> FullArgs;
   FullArgs.push_back(Program);
   FullArgs.append(Args.begin(), Args.end());
-  std::string Result = flattenWindowsCommandLine(FullArgs);
-  return (Result.size() + 1) <= MaxCommandStringLength;
+  auto Result = flattenWindowsCommandLine(FullArgs);
+  assert(!Result.getError());
+  return (Result->size() + 1) <= MaxCommandStringLength;
 }
 }

llvm/unittests/Support/CommandLineTest.cpp

for (auto *S : cl::getRegisteredSubcommands()) {
     }
   }
   cl::ResetCommandLineParser();
 }
 
 TEST(CommandLineTest, ArgumentLimit) {
   std::string args(32 * 4096, 'a');
   EXPECT_FALSE(llvm::sys::commandLineFitsWithinSystemLimits("cl", args.data()));
+  std::string args2(256, 'a');
+  EXPECT_TRUE(llvm::sys::commandLineFitsWithinSystemLimits("cl", args2.data()));
+  if (Triple(sys::getProcessTriple()).isOSWindows()) {
+    // We use 32000 as a limit for command line length. Program name ('cl'),
+    // separating spaces and termination null character occupy 5 symbols.
+    std::string long_arg(32000 - 5, 'b');
+    EXPECT_TRUE(
+        llvm::sys::commandLineFitsWithinSystemLimits("cl", long_arg.data()));
+    long_arg += 'b';
+    EXPECT_FALSE(
+        llvm::sys::commandLineFitsWithinSystemLimits("cl", long_arg.data()));
+  }
 }
 
 TEST(CommandLineTest, ResponseFileWindows) {
   if (!Triple(sys::getProcessTriple()).isOSWindows())
     return;
 
   StackOption<std::string, cl::list<std::string>> InputFilenames(
       cl::Positional, cl::desc("<input files>"), cl::ZeroOrMore);