Page MenuHomePhabricator

[Windows] Fix limit on command line size
ClosedPublic

Authored by sepavloff on Jul 14 2020, 7:22 AM.

Details

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.

Diff Detail

Event Timeline

sepavloff created this revision.Jul 14 2020, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 7:22 AM

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 updated this revision to Diff 278117.Jul 15 2020, 2:28 AM

Addressed reviewer's notes

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 updated this revision to Diff 278132.Jul 15 2020, 3:46 AM

Fixed typo

amccarth accepted this revision.Jul 15 2020, 8:15 AM

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.

This revision is now accepted and ready to land.Jul 15 2020, 8:15 AM
sepavloff updated this revision to Diff 278221.Jul 15 2020, 9:33 AM

Fixed unit test

This revision was automatically updated to reflect the committed changes.

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)

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 removed a subscriber: max-kudr.Jul 21 2020, 3:06 PM
sepavloff reopened this revision.Jul 21 2020, 11:27 PM
This revision is now accepted and ready to land.Jul 21 2020, 11:27 PM

Added changes for LLDB

Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 11:28 PM
sepavloff added subscribers: max-kudr, k8stone.

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

This revision was automatically updated to reflect the committed changes.