Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Report: The conhost command line is not properly escaped #1090

Closed
fcharlie opened this issue Jun 1, 2019 · 10 comments

Comments

@fcharlie
Copy link
Contributor

commented Jun 1, 2019

Environment

Windows build number: [run "ver" at a command prompt]
Windows Terminal version (if applicable):

Any other software?

Today I modified the Windows Terminal configuration file and wanted to create a Pwsh with the emoji title. However, I encountered some confusion in this process. I checked the task manager and found that conhost did not escape when the process was started, causing the command line to parse the exception.

Steps to reproduce

Expected behavior

I need to start pwsh by the following command line (note that the following command line is in cmd, or you can start pwsh normally using CreateProcessW.)

"C:\Program Files\PowerShell\7-preview\pwsh.exe" -NoExit -Command "$Host.UI.RawUI.WindowTitle=\"Windows Pwsh 💙 (7 Preview)\""

Here is the Windows Terminal configuration file (the command line has been properly escaped):

        {
            "startingDirectory": "C:\\Users\\CharlieInc",
            "guid": "{08a0be98-ff68-4e3a-a054-0fbd3969d3bb}",
            "name": "Windows Pwsh 💙 (7 Preview)",
            "colorscheme": "Campbell",
            "historySize": 9001,
            "snapOnInput": true,
            "cursorColor": "#FFFFFF",
            "cursorShape": "bar",
            "commandline": "\"C:\\Program Files\\PowerShell\\7-preview\\pwsh.exe\" -NoExit -Command \"$Host.UI.RawUI.WindowTitle=\\\"Windows Pwsh 💙 (7 Preview)\\\"\"",
            "fontFace": "Consolas",
            "fontSize": 12,
            "acrylicOpacity": 0.75,
            "useAcrylic": true,
            "closeOnExit": false,
            "padding": "0, 0, 0, 0",
            "icon": "ms-appdata:///roaming/pwsh-32.png"
        }

Actual behavior

As expected, Pwsh should set the title correctly, but pwsh reported the error:
image

Let's take a look at the command line:

image

image

When you see the red line, the command line that pwsh starts is incorrect, and when you start conhost, the command line is still correct!

Currently I have found a suspicious code, most likely this code caused an error:

HRESULT ConsoleArguments::_GetClientCommandline(_Inout_ std::vector<std::wstring>& args, const size_t index, const bool skipFirst)
{
auto start = args.begin()+index;
// Erase the first token.
// Used to get rid of the explicit commandline token "--"
if (skipFirst)
{
// Make sure that the arg we're deleting is "--"
FAIL_FAST_IF(!(CLIENT_COMMANDLINE_ARG == start->c_str()));
args.erase(start);
}
_clientCommandline = L"";
size_t j = 0;
for (j = index; j < args.size(); j++)
{
_clientCommandline += args[j];
if (j+1 < args.size())
{
_clientCommandline += L" ";
}
}
args.erase(args.begin()+index, args.begin()+j);
return S_OK;

In this code, command line composition is simply a simple connection rather than a concatenated string. If the string contains spaces, this will result in an incorrect command line.

Below is a code for the command line escaping, which may be useful:

inline std::wstring escape_argument(std::wstring_view ac) {
  if (ac.empty()) {
    return L"\"\"";
  }
  bool hasspace = false;
  auto n = ac.size();
  for (auto c : ac) {
    switch (c) {
    case L'"':
    case L'\\':
      n++;
      break;
    case ' ':
    case '\t':
      hasspace = true;
      break;
    default:
      break;
    }
  }
  if (hasspace) {
    n += 2;
  }
  if (n == ac.size()) {
    return std::wstring(ac.data(), ac.size());
  }
  std::wstring buf;
  if (hasspace) {
    buf.push_back(L'"');
  }
  size_t slashes = 0;
  for (auto c : ac) {
    switch (c) {
    case L'\\':
      slashes++;
      buf.push_back(L'\\');
      break;
    case L'"': {
      for (; slashes > 0; slashes--) {
        buf.push_back(L'\\');
      }
      buf.push_back(L'\\');
      buf.push_back(c);
    } break;
    default:
      slashes = 0;
      buf.push_back(c);
      break;
    }
  }
  if (hasspace) {
    for (; slashes > 0; slashes--) {
      buf.push_back(L'\\');
    }
    buf.push_back(L'"');
  }
  return buf;
}
@zadjii-msft

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I'm inclined to think that this will be solved when we switch to the real conpty API. Right now, we're using the legacy method of launching conhost.exe as a conpty, and requesting that conhost launches the process on behalf of us. This results in the commandline needing to be escaped twice like this, which is pretty painful. We have some work planned to switch the Terminal back to using the API directly. When we make that change, then we won't need to double-escape the commandline, we'll just use the commandline to CreateProcess the process directly, and we'll attach that process to a conpty.

@DHowett-MSFT

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@zadjii-msft is totally right. I'll file up with a Deliverable to use *PseudoConsole and tack that on to the Deliverable for make pseudoconsole API a framework package or something?.

@fcharlie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@zadjii-msft @DHowett-MSFT Thanks.

Using the conpty API is correct, but I think conhost still needs to solve this problem.

@DHowett-MSFT

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

conhost still needs to solve this problem

It will probably solve this problem by getting 100% out of the business of accepting commandlines of other applications to spawn as arguments.

@fcharlie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@zadjii-msft

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

(adding some notes from a related email thread:)


I'm hacking some integration between Windows Terminal and a new VS2019 feature but this doesn't work (at least not obviously/easily/with normal json)

"commandline": "C:\\Windows\\SysWOW64\\WindowsPowerShell\\v1.0\\powershell.exe -ExecutionPolicy Bypass -NoLogo -NoExit -File \"C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Preview\\Common7\\Tools\\VsDevPs.ps1\"",

image


Okay, so the problem is in conhost. When we’re starting conhost for the Terminal, we get the commandline already broken up into args. Conhost then takes all those args, and joins them with spaces, to call CreateProcess again, to launch the client app. Because CreateProcess has already tried to help here while launching `conhost -- `, it already grouped the entire last arg as a single arg. So, conhost never knew that last arg had quotes around it.

If you once again escape the quotes around the path, it should work:

"commandline": "C:\\Windows\\SysWOW64\\WindowsPowerShell\\v1.0\\powershell.exe -ExecutionPolicy Bypass -NoLogo -NoExit -File \\\"C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Preview\\Common7\\Tools\\VsDevPs.ps1\\\"",

image


@fcharlie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@zadjii-msft I still recommend reassembling the command line arguments for use by CreateProcess using the previous escape_argument

@msftbot msftbot bot added the In-PR label Jul 4, 2019

@fcharlie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@zadjii-msft @DHowett-MSFT
PR #1815 should fix this problem.

image

image

@fcharlie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Unfortunately, Windows UCRT's spawnvp spawnv and other functions do not correctly handle escaping command line arguments, which is the same as conhost.

Can someone feedback this question to the ucrt team?

See: C:\Program Files (x86)\Windows Kits\10\Source\10.0.18362.0\ucrt\exec\cenvarg.cpp +24

// Converts a main()-style argv arguments vector into a command line.  On success,
// returns a pointer to the newly constructed arguments block; the caller is
// responsible for freeing the string.  On failure, returns null and sets errno.
template <typename Character>
static errno_t __cdecl construct_command_line(
    Character const* const* const argv,
    Character**             const command_line_result
    ) throw()
{
    typedef __crt_char_traits<Character> traits;

    *command_line_result = nullptr;

    // Compute the number of bytes required to store the arguments in argv in a
    // command line string (including spaces between arguments and a terminator):
    size_t const command_line_count = [&]
    {
        size_t n = 0;
        for (Character const* const* it = argv; *it; n += traits::tcslen(*it++) + 1) { }

        // If there were no arguments, return 1 so that we can return an empty
        // string:
        return __max(n, 1);
    }();

    __crt_unique_heap_ptr<Character> command_line(_calloc_crt_t(Character, command_line_count));
    if (!command_line)
    {
        __acrt_errno_map_os_error(ERROR_NOT_ENOUGH_MEMORY);
        return errno = ENOMEM;
    }

    Character const* const* source_it = argv;
    Character*              result_it = command_line.get();

    // If there are no arguments, just return the empty string:
    if (*source_it == '\0')
    {
        *command_line_result = command_line.detach();
        return 0;
    }

    // Copy the arguments, separated by spaces:
    while (*source_it != '\0')
    {
        _ERRCHECK(traits::tcscpy_s(result_it, command_line_count - (result_it - command_line.get()), *source_it));
        result_it += traits::tcslen(*source_it);
        *result_it++ = ' ';
        ++source_it;
    }

    // Replace the last space with a terminator:
    result_it[-1] = '\0';

    *command_line_result = command_line.detach();
    return 0;
}
@msftbot

This comment has been minimized.

Copy link

commented Aug 3, 2019

🎉This issue was addressed in #1815, which has now been successfully released as Windows Terminal Preview v0.3.2142.0.🎉

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.