Re: [PATCH v2] Cygwin: fork: Call pthread::atforkprepare() in lock_pthread()

Hi Takashi,

On Wed, 20 May 2026, Takashi Yano wrote:

> Since the commit 5f515cf3d6e3, if one thread calls fclose() while
> another thread calls fork(), a deadlock can occur. The mechanism
> is as follows.
>   1) fclose() first calls __sfp_lock_acquire() and, then calls
>      lock_process::locker.acquire() via cygheap_fdget().
>   2) fork() first calls lock_process::locker.acuire() via the
>      constructor of the lock_process class in the hold_everything
>      class, and then calls __sfp_lock_acquire() via __fp_lock_all()
>      in atforkprepare().
>   3) As a result, the thread calling fclose() tries to acquire the
>      lock_process lock while holding __sfp_lock, and the thread
>      calling fork() tries to acquire __sfp_lock while holding the
>      lock_process lock.
> This leads to a deadlock. Before the commit 5f515cf3d6e3, __sfp_lock
> was acquired in the constructor of the lock_pthread class in the
> hold_everything class, and since lock_pthread is defined before
> lock_process, this deadlock did not occur.
> 
> This patch moves the atforkprepare() call back into the constructor
> of the lock_pthread class, restoring the previous lock qcquisition
> order.

I am not super familiar with this part of the code, which is why this
commit message is really helpful. From my point of view, this patch is
good to go.

Thanks,
Johannes

P.S.: You may want to s/qcquisition/qcquisition/ before applying, even if
this typo cannot harm the clarity of the commit message.

> 
> Fixes: 5f515cf3d6e3 ("Cygwin: add _Fork() system call per POSIX.1-2024")
> Signed-off-by: Takashi Yano <[email protected]>
> Reviewed-by:
> ---
> v2: Fix the commit title
> 
>  winsup/cygwin/local_includes/sigproc.h | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/winsup/cygwin/local_includes/sigproc.h b/winsup/cygwin/local_includes/sigproc.h
> index 92cda94dc..21367877c 100644
> --- a/winsup/cygwin/local_includes/sigproc.h
> +++ b/winsup/cygwin/local_includes/sigproc.h
> @@ -131,7 +131,15 @@ class lock_pthread
>  {
>    bool bother;
>  public:
> -  lock_pthread (): bother (1) {}
> +  lock_pthread (bool do_atfork_handlers): bother (1)
> +  {
> +    /* POSIX.1-2024: _Fork() does not call any handler established
> +		     by pthread_atfork(). */
> +    if (do_atfork_handlers)
> +      dont_bother ();
> +    else
> +      prepare ();
> +  }
>    void prepare ()
>    {
>      pthread::atforkprepare ();
> @@ -166,15 +174,8 @@ class hold_everything
>    lock_process process;
>  
>  public:
> -  hold_everything (bool& x, bool do_atfork_handlers): ischild (x)
> -  {
> -    /* POSIX.1-2024: _Fork() does not call any handler established
> -		     by pthread_atfork(). */
> -    if (do_atfork_handlers)
> -      pthread.dont_bother ();
> -    else
> -      pthread.prepare ();
> -  }
> +  hold_everything (bool& x, bool do_atfork_handlers): ischild (x),
> +  pthread (do_atfork_handlers) {}
>    operator int () const {return signals;}
>  
>    ~hold_everything()
> -- 
> 2.51.0
> 
>