[PATCH v5 1/3] Cygwin: pty: Use OpenConsole.exe if available

[Takashi Yano]

Hi Johannes,

Thanks for reviewing!

On Mon, 16 Mar 2026 10:55:29 +0100 (CET)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> On Thu, 12 Mar 2026, Takashi Yano wrote:
> 
> > This patch replaces legacy conhost.exe with OpenConsole.exe if
> > it is available. This enables various new features such as mouse
> > support in pseudo console and bug fixes. The legacy conhost has
> > problems, e.g. character attributes are mangled or ignored, and
> > terminal reports are not passed through. This patch resolve the
> > issue by loading /usr/bin/OpenColnsole.exe instead of conhost.exe
> 
> I know that Corinna cares about typos in commit messages, so I'd like to
> point out that "OpenColnsole" has an extra "l" in it.
> 
> > if it is available.
> 
> My biggest issue with this patch: There is no opt-out mechanism: No
> CYGWIN=disable_openconsole or MSYS=disable_openconsole flag. If
> /usr/bin/OpenConsole.exe exists and is buggy, or this here patch, the user
> is stuck.

Thnaks. I'll add CYGWIN=use_lagacy_pcon option.

> My second-biggest issue: The commit message leaves too many legitimate
> questions unanswered. It says "various new features such as mouse support"
> and "terminal reports are not passed through" without specifics, without

To be honest, aside from the few points I mentioned here, I’m not aware
of any other improvements in OpenConsole.exe.
I tested only mouse support and attached text from Thomas.
These improvements seems already applied to conhost.exe in
latest Windows 11.

Any idea > Thomas?

> linking to any bug report, and without explaining why reimplementing
> `CreatePseudoConsole()` using undocumented NT kernel APIs
> (`\Device\ConDrv\Server`) is necessary (vs. just passing the
> `OpenConsole.exe` path to the standard `CreatePseudoConsole()` somehow).
> The message also doesn't address the maintenance burden of vendoring
> Windows Terminal code.

I expect the APIs will not be changed so often, because the same code
works even for old conhost.exe.

> Another concern I have, which is however very easily addressed, is that
> this patch does two things for the price of one. It would be better to
> separate the CSIc handling from the `OpenConsole.exe` integration. This
> would make it possible to fast-track one without the other, and make
> reviews of future iterations much easier.

I'll separate the CSIc matter.

> > +extern "C" WINBASEAPI HRESULT WINAPI
> 
> This should probably not be exported from cygwin1.dll, it is for
> internal use only.
> 
> > +CreatePseudoConsole_new (COORD size, HANDLE h_input, HANDLE h_output,
> > +			 DWORD flags, HPCON *hpcon)

Thanks. Fixed.

> > +{
> > +
> > +  HANDLE h_con_server, h_con_reference;
> > +  NTSTATUS status;
> > +  BOOL res;
> > +  HANDLE h_read_pipe, h_write_pipe;
> > +  BOOL inherit_cursor;
> > +  path_conv conhost ("/usr/bin/OpenConsole.exe");
> 
> Is it really a good idea to hard-code that path? That would not only
> preclude an `OpenConsole.exe` that is in the `PATH` to be used, it also
> suggests that you plan on building a Cygwin version of that executable
> (because that location should only contain native Cygwin applications and
> DLLs).

I imagine we would have openconsole cygwin package which install
OpenConsole.exe into /usr/bin/.

> > +  InitializeProcThreadAttributeList (NULL, 1, 0, &list_size);
> 
> This requires a corresponding `DeleteProcThreadAttributeList()` call.

This call is for retrieving list_size. Nothing is allocated with this call.
 
> > +  hpcon_internal->hWritePipe = h_write_pipe;
> > +  hpcon_internal->hConDrvReference = h_con_reference;
> > +  hpcon_internal->hConHostProcess = pi.hProcess;
> > +  *hpcon = (HPCON) hpcon_internal;
> > +
> > +  HeapFree (GetProcessHeap(), 0, attr_list);
> > +  CloseHandle (h_con_server);
> > +  CloseHandle (pi.hThread);
> 
> I see `h_read_pipe` being closed in the failure mode, but not in the
> success one.
> 
> `h_write_pipe` is stored in `*h_pcon`, so it becomes the caller's
> responsibility. And `h_con_server`/`pi.hThread` are closed properly, but I
> don't see where `h_read_pipe` is handled properly.

Indeed. My fault. Fixed.

> > -  if (get_ttyp ()->pcon_start)
> > +  int pcon_start_mode =
> > +    get_ttyp ()->pcon_start ? 1 : (get_ttyp ()->pcon_start_csi_c ? 2 : 0);
> > +  if (pcon_start_mode)
> 
> Does this need to be made thread-safe in case `write()` is called
> simultaneously on two separate processor cores?

Yeah! Thanks for pointing this out. This is not a bug only of
OpenConsole.exe implementation, but also regacy psseudo console.
I'll submit an indivisual patch.

> > @@ -2693,6 +2865,7 @@ workarounds_for_pseudo_console_output (char *outbuf, DWORD rlen)
> >    int arg = 0;
> >    bool saw_greater_than_sign = false;
> >    bool saw_question_mark = false;
> > +  static bool in_pcon_start = false;
> >    for (DWORD i=0; i<rlen; i++)
> >      if (state == 0 && outbuf[i] == '\033')
> >        {
> > @@ -2774,8 +2947,21 @@ workarounds_for_pseudo_console_output (char *outbuf, DWORD rlen)
> >  	    start_at = i;
> >  	    state = 1;
> >  	  }
> > +	else if (arg == 6 && outbuf[i] == 'n' && ttyp->pcon_start)
> > +	  {
> > +	    in_pcon_start = true;
> 
> Should this variable be reset in error paths below? This might be
> _particularly_ nasty to debug because `in_pcon_start` is `static` and will
> therefore persist indefinitely.

Do you mean in the else block:
    else
      { /* Never reached */
        is_csi = false;
        is_osc = false;
        saw_greater_than_sign = false;
        saw_question_mark = false;
        arg = 0;
        state = 0;
      }
?

Maybe. Added.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xtextattr1
Type: application/octet-stream
Size: 390 bytes
Desc: not available
URL: <https://cygwin.com/pipermail/cygwin-patches/attachments/20260325/26fffcc1/attachment.obj>