[PATCH v3] Cygwin: console: Abort setting disable_master_thread when no con.owner

[Takashi Yano]

Hi Johannes,

On Fri, 18 Sep 2026 17:50:54 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> On Thu, 17 Sep 2026, Takashi Yano wrote:
> 
> > With the commit 733d5a953fa9 ("Cygwin: console: Ensure the master
> > thread runs only when it is supposed to"), the process which calls
> > set_disable_master_thread() hangs if the con.owner already exited,
> > because set_disable_master_thread() waits for cons_master_thread
> > accepting the status change and reflecting the current status to
> > master_thread_suspended. With this patch, set_disable_master_thread()
> > is aborted if the owner process no longer exists to avoid this
> > hang.
> > 
> > Addresses: https://cygwin.com/pipermail/cygwin/2026-September/260037.html
> > Fixes: 733d5a953fa9 ("Cygwin: console: Ensure the master thread runs only when it is supposed to")
> > Reported-by: Jay Libove Alzina <libove@felines.org>
> > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > ---
> > v2: Treat the absence of the master thread the same as suspension.
> > v3: Handle the crash of owner process.
> 
> Thank you!
> 
> There's one more concern I have: An owner query failure does not
> necessarily mean the process exited, and observing exit is not a worker
> acknowledgement.
> 
> I think we need to allow an owner-exit escape only on observed exit or
> absence, with con.owner still matching the captured owner. We need to
> leave the acknowledgement untouched and preserve -1 as normal retirement,
> and log inconclusive query or wait failures once per newly observed
> failing owner and keep waiting.
> 
> More concretely, I am worried about this scenario: An old owner's absence
> can overwrite a replacement's acknowledgement. Non-owner cleanup A holds
> `cons_mode_mutex` and requests resume, while acknowledgement remains true.
> Process A captures a dead owner's PID or zero and selects the escape
> branch before reacquiring `input_mutex`. Meanwhile, process B can claim
> ownership in `open()` and start its worker before obtaining the mode
> mutex, as v17 deliberately permits. Process B's worker sees the retained
> false request and acknowledges resume. Process A then obtains the input
> lock but rechecks neither owner nor acknowledgement before writing true.
> The shared record now calls process B suspended despite its
> acknowledgement. Later mode decisions use that record when selecting
> `ENABLE_PROCESSED_INPUT`. Holding the mode mutex does not exclude the new
> startup path, and taking the input lock does not validate the earlier
> owner observation.

Sorry, I don't get your point. What do you mean by 'worker'? The
caller of set_disable_master_thread()?

(1)
I think it’s actually safer to mistakenly treat the owner as dead
when it’s still alive than to make the opposite mistake. Therefore
I use:
`if (owner == 0 || owner == (DWORD) -1 || !process_alive (owner))`
owner == 0: No master thread
owner == -1: The master thread is about to exit.
!process_alive (owner): Possibly, the owner process died.

(2)
Setting con.master_thread_suspended = true is necessary when the
master_thread is not respoiding, because Ctrl-C should work at
least even when the master_thread is not alive and the process
does not call process_input_message().
If master_thread_suspended is not set, the set_input_mode()
may clears ENABLE_PROCESSED_INPUT, so Ctrl-C does not work because
of absence of the master thread.

Your code seems to be trying to track the liveness of the current
master thread as accurately as possible, but I’m not sure that level
of accuracy is really necessary.

If we mistakenly wait for an acknowledgement, it can cause a hang.
But if we mistakenly don’t wait for an acknowledgement, what kind
of risks do you think that would introduce?

> A related concern: Retiring is not the same as retired. `close()` sets
> owner to `-1`, waits for the worker's completion event, then publishes
> zero. V3 treats that intermediate value as absence.
> 
> Another note: `process_alive()` also returns false when opening or
> querying a process fails. A failed query is not verified process exit.
> This distinction needs deliberate error handling, not an unconditional
> assertion that the worker no longer exists.
> 
> I'd like to offer this as a discussion starter:
> 
> -- snip --
> diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
> index 16c3e5270a..39352ddda1 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -4950,16 +4950,40 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
>    cons->acquire_input_mutex (mutex_timeout);
>    con.disable_master_thread = x;
>    cons->release_input_mutex ();
> +  DWORD reported_owner = 0;
>    while (con.master_thread_suspended != x)
> -    { /* Wait for the responce from the cons_master_thread. */
> +    {
>        DWORD owner = con.owner;
> -      if (owner == 0 || owner == (DWORD) -1 || !process_alive (owner))
> -	{ /* The process that runs cons_master_thread no longer exists. */
> -	  cons->acquire_input_mutex (mutex_timeout);
> -	  /* Treat the absence of the master_thread the same as suspension. */
> -	  con.master_thread_suspended = true;
> -	  cons->release_input_mutex ();
> -	  return; /* Abort */
> +      if (owner != (DWORD) -1)
> +	{
> +	  bool exited = owner == 0;
> +	  DWORD error = ERROR_SUCCESS;
> +	  if (owner)
> +	    {
> +	      HANDLE process = OpenProcess (SYNCHRONIZE, FALSE, owner);
> +	      if (process)
> +		{
> +		  DWORD res = WaitForSingleObject (process, 0);
> +		  if (res == WAIT_FAILED)
> +		    error = GetLastError ();
> +		  exited = res == WAIT_OBJECT_0;
> +		  CloseHandle (process);
> +		}
> +	      else
> +		{
> +		  error = GetLastError ();
> +		  exited = error == ERROR_INVALID_PARAMETER;
> +		}
> +	    }
> +	  /* Keep the request, but never invent a worker acknowledgement. */
> +	  if (exited && con.owner == owner)
> +	    return;
> +	  if (error && !exited && reported_owner != owner)
> +	    {
> +	      system_printf ("Cannot query console owner %u, error %u",
> +			     owner, error);
> +	      reported_owner = owner;
> +	    }
>  	}
>        Sleep (1);
>      }
> -- snap --
> 
> I haven't been able to fully wrap my head around these issues, and need to
> unwind this weekend. Hopefully you can turn this into a correct and
> complete fix.
> 
> Thank you,
> Johannes
> 
> > 
> >  winsup/cygwin/fhandler/console.cc | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
> > index a877fce0b..0b9e205e6 100644
> > --- a/winsup/cygwin/fhandler/console.cc
> > +++ b/winsup/cygwin/fhandler/console.cc
> > @@ -5025,7 +5025,18 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
> >    con.disable_master_thread = x;
> >    cons->release_input_mutex ();
> >    while (con.master_thread_suspended != x)
> > -    Sleep (1);
> > +    { /* Wait for the responce from the cons_master_thread. */
> > +      DWORD owner = con.owner;
> > +      if (owner == 0 || owner == (DWORD) -1 || !process_alive (owner))
> > +	{ /* The process that runs cons_master_thread no longer exists. */
> > +	  cons->acquire_input_mutex (mutex_timeout);
> > +	  /* Treat the absence of the master_thread the same as suspension. */
> > +	  con.master_thread_suspended = true;
> > +	  cons->release_input_mutex ();
> > +	  return; /* Abort */
> > +	}
> > +      Sleep (1);
> > +    }
> >  }
> >  
> >  int
> > -- 
> > 2.51.0
> > 
> > 


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>