CreatePseudoConsole doesn't handle sockets correctly

[DaanDeMeyer]

The documentation of CreatePseudoConsole explicitly documents that CreatePseudoConsole can be used with sockets:

https://docs.microsoft.com/en-us/windows/console/creating-a-pseudoconsole-session#preparing-the-communication-channels

If we look at the implementation of CreatePseudoConsole, we can see it calls DuplicateHandle on the handles passed to it:

terminal/src/winconpty/winconpty.cpp

Lines 348 to 349 in 2712e41

	RETURN_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), hInput, GetCurrentProcess(), duplicatedInput.addressof(), 0, TRUE, DUPLICATE_SAME_ACCESS));
	RETURN_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), hOutput, GetCurrentProcess(), duplicatedOutput.addressof(), 0, TRUE, DUPLICATE_SAME_ACCESS));

However, the documentation of DuplicateHandle explicitly mentions that DuplicateHandle should not be used to duplicate socket handles:

https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-duplicatehandle#remarks

Doesn't this make the CreatePseudoConsole implementation incorrect when using sockets?

I haven't actually confirmed whether sockets work or not, I'm currently trying to get CreatePseudoConsole to work with sockets but it's returning Access is Denied. While looking at the source code to figure out why it's returning Access is Denied, I stumbled upon this possible issue.

EDIT: Nvm the Access is Denied issue, I incorrectly assumed CreatePseudoConsole would return 0 on failure. The DuplicateHandle issue still stands however.

EDIT2: Removed edit because it was a programming error by me.

[DaanDeMeyer]

Reproduce that shows WSAPoll doesn't return when the sockets are closed when passing sockets to CreatePseudoConsole:

#define _WIN32_WINNT _WIN32_WINNT_WIN10

#include <assert.h>
#include <stdbool.h>
#include <stdio.h>
#include <winsock2.h>

enum { ERROR_STRING_MAX_SIZE = 512 };

static DWORD error_from_hresult(HRESULT error)
{
  if ((error & (long) 0xFFFF0000) ==
      MAKE_HRESULT(SEVERITY_ERROR, FACILITY_WIN32, 0)) {
    return HRESULT_CODE(error);
  }

  if (error == S_OK) {
    return ERROR_SUCCESS;
  }

  // Not a Win32 HRESULT so return a generic error code.
  return ERROR_CAN_NOT_COMPLETE;
}

static const char *error_string(int error)
{
  static wchar_t *wstring = NULL;
  int r = 0;

  wstring = malloc(sizeof(wchar_t) * ERROR_STRING_MAX_SIZE);
  if (wstring == NULL) {
    return "Failed to allocate memory for error string";
  }

  // We don't expect message sizes larger than the maximum possible int.
  r = (int) FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM |
                               FORMAT_MESSAGE_IGNORE_INSERTS,
                           NULL, (DWORD) abs(error),
                           MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), wstring,
                           ERROR_STRING_MAX_SIZE, NULL);
  if (r == 0) {
    free(wstring);
    return "Failed to retrieve error string";
  }

  static char string[ERROR_STRING_MAX_SIZE];

  r = WideCharToMultiByte(CP_UTF8, 0, wstring, -1, string,
                          ERROR_STRING_MAX_SIZE, NULL, NULL);

  free(wstring);

  if (r == 0) {
    return "Failed to convert error string to UTF-8";
  }

  // Remove trailing whitespace and period.
  if (r >= 4) {
    string[r - 4] = '\0';
  }

  return string;
}

static int error_unify_or_else(int r, int success)
{
  assert(GetLastError() <= INT_MAX);

  if (r < -1) {
    return r;
  }

  if (r == -1) {
    return -WSAGetLastError();
  }

  if (r == 0) {
    return -(int) GetLastError();
  }

  return success;
}

static int error_unify(int r)
{
  return error_unify_or_else(r, 0);
}

static int init(void)
{
  WSADATA data;
  int r = WSAStartup(MAKEWORD(2, 2), &data);
  return error_unify(r);
}

static SOCKET socket_destroy(SOCKET socket)
{
  if (socket == INVALID_SOCKET) {
    return INVALID_SOCKET;
  }

  closesocket(socket);

  return INVALID_SOCKET;
}

static int socket_nonblocking(SOCKET socket, bool enable)
{
  u_long mode = enable;
  int r = ioctlsocket(socket, (long) FIONBIO, &mode);
  return error_unify(r);
}

static int socketpair(int domain, int type, int protocol, SOCKET *out)
{
  SOCKET server = INVALID_SOCKET;
  SOCKET pair[] = { INVALID_SOCKET, INVALID_SOCKET };
  int r = -1;

  server = WSASocketW(AF_INET, SOCK_STREAM, 0, NULL, 0, 0);
  if (server == INVALID_SOCKET) {
    goto finish;
  }

  SOCKADDR_IN localhost = { 0 };
  localhost.sin_family = AF_INET;
  localhost.sin_addr.S_un.S_addr = htonl(INADDR_LOOPBACK);
  localhost.sin_port = 0;

  r = bind(server, (SOCKADDR *) &localhost, sizeof(localhost));
  if (r < 0) {
    goto finish;
  }

  r = listen(server, 1);
  if (r < 0) {
    goto finish;
  }

  SOCKADDR_STORAGE name = { 0 };
  int size = sizeof(name);
  r = getsockname(server, (SOCKADDR *) &name, &size);
  if (r < 0) {
    goto finish;
  }

  pair[0] = WSASocketW(domain, type, protocol, NULL, 0, 0);
  if (pair[0] == INVALID_SOCKET) {
    goto finish;
  }

  r = socket_nonblocking(pair[0], true);
  if (r < 0) {
    goto finish;
  }

  r = connect(pair[0], (SOCKADDR *) &name, size);
  if (r < 0 && WSAGetLastError() != WSAEWOULDBLOCK) {
    goto finish;
  }

  r = socket_nonblocking(pair[0], false);
  if (r < 0) {
    goto finish;
  }

  pair[1] = accept(server, NULL, NULL);
  if (pair[1] == INVALID_SOCKET) {
    r = -1;
    goto finish;
  }

  out[0] = pair[0];
  out[1] = pair[1];

  pair[0] = INVALID_SOCKET;
  pair[1] = INVALID_SOCKET;

  r = 0;

finish:
  socket_destroy(server);
  socket_destroy(pair[0]);
  socket_destroy(pair[1]);

  return error_unify(r);
}

static int socket_init(SOCKET *parent, SOCKET *child, bool input)
{
  assert(parent);
  assert(child);

  SOCKET pair[] = { INVALID_SOCKET, INVALID_SOCKET };
  int r = 0;

  r = socketpair(AF_INET, SOCK_STREAM, 0, pair);
  if (r < 0) {
    goto finish;
  }

  r = SetHandleInformation((HANDLE) pair[0], HANDLE_FLAG_INHERIT, 0);
  if (r == 0) {
    goto finish;
  }

  r = SetHandleInformation((HANDLE) pair[1], HANDLE_FLAG_INHERIT, 0);
  if (r == 0) {
    goto finish;
  }

  // Make the connection unidirectional to better emulate a pipe.

  r = shutdown(pair[0], SD_SEND);
  if (r < 0) {
    goto finish;
  }

  r = shutdown(pair[1], SD_RECEIVE);
  if (r < 0) {
    goto finish;
  }

  *parent = input ? pair[1] : pair[0];
  *child = input ? pair[0] : pair[1];

  pair[0] = INVALID_SOCKET;
  pair[1] = INVALID_SOCKET;

finish:
  socket_destroy(pair[0]);
  socket_destroy(pair[1]);

  return error_unify(r);
}

static LPPROC_THREAD_ATTRIBUTE_LIST setup_attribute_list(HPCON console)
{
  int r = 0;

  // Get the required size for `attribute_list`.
  SIZE_T attribute_list_size = 0;
  r = InitializeProcThreadAttributeList(NULL, 1, 0, &attribute_list_size);
  if (r == 0 && GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
    return NULL;
  }

  LPPROC_THREAD_ATTRIBUTE_LIST attribute_list = malloc(attribute_list_size);
  if (attribute_list == NULL) {
    SetLastError(ERROR_NOT_ENOUGH_MEMORY);
    return NULL;
  }

  r = InitializeProcThreadAttributeList(attribute_list, 1, 0,
                                        &attribute_list_size);
  if (r == 0) {
    free(attribute_list);
    return NULL;
  }

  r = UpdateProcThreadAttribute(attribute_list, 0,
                                PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, console,
                                sizeof(console), NULL, NULL);
  if (r == 0) {
    DeleteProcThreadAttributeList(attribute_list);
    return NULL;
  }

  return attribute_list;
}

int main(void)
{
  SOCKET input[2] = { INVALID_SOCKET, INVALID_SOCKET };
  SOCKET output[2] = { INVALID_SOCKET, INVALID_SOCKET };
  LPPROC_THREAD_ATTRIBUTE_LIST attribute_list = NULL;
  int r = -1;

  r = init();
  if (r < 0) {
    return EXIT_FAILURE;
  }

  r = socket_init(&input[0], &input[1], true);
  if (r < 0) {
    goto finish;
  }

  r = socket_init(&output[0], &output[1], false);
  if (r < 0) {
    goto finish;
  }

  COORD size = { 100, 100 };
  HPCON console = INVALID_HANDLE_VALUE;
  r = CreatePseudoConsole(size, (HANDLE) input[1], (HANDLE) output[1], 0,
                          &console);
  if (r != S_OK) {
    SetLastError(error_from_hresult(r));
    r = error_unify(0);
    goto finish;
  }

  attribute_list = setup_attribute_list(console);
  if (attribute_list == NULL) {
    r = error_unify(0);
    goto finish;
  }

  STARTUPINFOEXA extended_startup_info = {
    .StartupInfo = { .cb = sizeof(extended_startup_info) },
    .lpAttributeList = attribute_list
  };

  LPSTARTUPINFOA startup_info_address = &extended_startup_info.StartupInfo;
  PROCESS_INFORMATION info;

  r = CreateProcessA(NULL, "echo test", NULL, NULL, true,
                     EXTENDED_STARTUPINFO_PRESENT, NULL, NULL,
                     startup_info_address, &info);
  if (r == 0) {
    r = error_unify(r);
    goto finish;
  }

  socket_destroy(input[1]);
  socket_destroy(output[1]);

  WSAPOLLFD pollfd = { .fd = output[0], .events = POLLIN };

  while (true) {
    r = WSAPoll(&pollfd, 1, 10000);
    if (r < 0) {
      r = error_unify(r);
      goto finish;
    }

    char buffer[4096];

    r = recv(output[0], buffer, 4096, 0);
    if (r < 0) {
      r = error_unify(r);
      goto finish;
    }

    if (r == 0) {
      break;
    }

    buffer[r] = '\0';

    printf("%s", buffer);
  }

finish:
  if (r < 0) {
    fprintf(stderr, "%s\n", error_string(r));
  }

  return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
}

Compiled with clang <file> -lws2_32. On my machine, this prints test and then blocks until the socket connection times out. I think in this scenario WSAPoll should return once the echo child process exits and the child endpoint of the output socket connection is closed.

[DaanDeMeyer]

If we add socket_destroy(input[0]); just before entering the while(true) loop, The ConHost instance starts consuming an entire CPU core as well.

[DHowett-MSFT]

Wow, thanks for figuring this out. I'm going to file a documentation bug and triage this one in for Product-Conpty.

[DHowett-MSFT]

Oisin already did that second thing, so I'll just do the first thing. Thanks @oising 😁

[DHowett-MSFT]

Doc tracking: MicrosoftDocs/Console-Docs#124