newlib: Unlock the mutex while calling atexit()'ed functions

  The atexit()'ed function may deadlock if it waits for another thread
which calls atexit() in the current __call_atexit.c code. This patch
unlock __atexit_recursive_mutex while calling atexit()'ed functions
to avoid the deadlock mentioned above. glibc and Darwin do the same,
so it sounds reasonable.

Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

Hi Takashi,

On Nov 18 22:31, Takashi Yano wrote:
> The atexit()'ed function may deadlock if it waits for another thread
> which calls atexit() in the current __call_atexit.c code. This patch
> unlock __atexit_recursive_mutex while calling atexit()'ed functions
> to avoid the deadlock mentioned above. glibc and Darwin do the same,
> so it sounds reasonable.
> 
> Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
> Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
> Reviewed-by:
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
> index 710440389..44f1f6acc 100644
> --- a/newlib/libc/stdlib/__call_atexit.c
> +++ b/newlib/libc/stdlib/__call_atexit.c
> @@ -114,6 +114,11 @@ __call_exitprocs (int code, void *d)
>  
>  	  ind = p->_ind;
>  
> +#ifndef __SINGLE_THREAD__
> +	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
> +	     deadlock if it waits for another thread which calls atexit(). */
> +	  __lock_release_recursive(__atexit_recursive_mutex);
> +#endif
>  	  /* Call the function.  */
>  	  if (!args || (args->_fntypes & i) == 0)
>  	    fn ();
> @@ -121,6 +126,9 @@ __call_exitprocs (int code, void *d)
>  	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
>  	  else
>  	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> +#ifndef __SINGLE_THREAD__
> +	  __lock_acquire_recursive(__atexit_recursive_mutex);
> +#endif
>  
>  	  /* The function we called call atexit and registered another
>  	     function (or functions).  Call these new functions before
> -- 
> 2.51.0

LGTM, please push.


Thanks,
Corinna
  

Hello Takashi Yano,

I have some questions to the change.

----- Am 18. Nov 2025 um 14:31 schrieb Takashi Yano takashi.yano@nifty.ne.jp:

> The atexit()'ed function may deadlock if it waits for another thread
> which calls atexit() in the current __call_atexit.c code. This patch
> unlock __atexit_recursive_mutex while calling atexit()'ed functions
> to avoid the deadlock mentioned above. glibc and Darwin do the same,
> so it sounds reasonable.
> 
> Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
> Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
> Reviewed-by:
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
> newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/newlib/libc/stdlib/__call_atexit.c
> b/newlib/libc/stdlib/__call_atexit.c
> index 710440389..44f1f6acc 100644
> --- a/newlib/libc/stdlib/__call_atexit.c
> +++ b/newlib/libc/stdlib/__call_atexit.c
> @@ -114,6 +114,11 @@ __call_exitprocs (int code, void *d)
> 
> 	  ind = p->_ind;
> 
> +#ifndef __SINGLE_THREAD__
> +	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
> +	     deadlock if it waits for another thread which calls atexit(). */
> +	  __lock_release_recursive(__atexit_recursive_mutex);
> +#endif

Here, a potentially shared structure is used through the args pointer. If some other thread calls exit() or atexit() concurrently, then this could result in a use of altered or freed memory. The data of the structure referenced by args should be first copied to local variables. Also I think that the deallocation should be done before we release the lock. We probably also have to restart the process after each handler call unconditionally.


> 	  /* Call the function.  */
> 	  if (!args || (args->_fntypes & i) == 0)
> 	    fn ();
> @@ -121,6 +126,9 @@ __call_exitprocs (int code, void *d)
> 	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
> 	  else
> 	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> +#ifndef __SINGLE_THREAD__
> +	  __lock_acquire_recursive(__atexit_recursive_mutex);
> +#endif
> 
> 	  /* The function we called call atexit and registered another
> 	     function (or functions).  Call these new functions before
> --
> 2.51.0
  

Hi Takashi, Hi Sebastian,

did you discuss this in PM, by any chance?  Is there any difference in
GLibc or Darwin which explains how this potential problem is fixed
there?


Thanks,
Corinna


On Nov 26 05:17, Sebastian Huber wrote:
> Hello Takashi Yano,
> 
> I have some questions to the change.
> 
> ----- Am 18. Nov 2025 um 14:31 schrieb Takashi Yano takashi.yano@nifty.ne.jp:
> 
> > The atexit()'ed function may deadlock if it waits for another thread
> > which calls atexit() in the current __call_atexit.c code. This patch
> > unlock __atexit_recursive_mutex while calling atexit()'ed functions
> > to avoid the deadlock mentioned above. glibc and Darwin do the same,
> > so it sounds reasonable.
> > 
> > Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
> > Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
> > Reviewed-by:
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> > newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> > 
> > diff --git a/newlib/libc/stdlib/__call_atexit.c
> > b/newlib/libc/stdlib/__call_atexit.c
> > index 710440389..44f1f6acc 100644
> > --- a/newlib/libc/stdlib/__call_atexit.c
> > +++ b/newlib/libc/stdlib/__call_atexit.c
> > @@ -114,6 +114,11 @@ __call_exitprocs (int code, void *d)
> > 
> > 	  ind = p->_ind;
> > 
> > +#ifndef __SINGLE_THREAD__
> > +	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
> > +	     deadlock if it waits for another thread which calls atexit(). */
> > +	  __lock_release_recursive(__atexit_recursive_mutex);
> > +#endif
> 
> Here, a potentially shared structure is used through the args pointer. If some other thread calls exit() or atexit() concurrently, then this could result in a use of altered or freed memory. The data of the structure referenced by args should be first copied to local variables. Also I think that the deallocation should be done before we release the lock. We probably also have to restart the process after each handler call unconditionally.
> 
> 
> > 	  /* Call the function.  */
> > 	  if (!args || (args->_fntypes & i) == 0)
> > 	    fn ();
> > @@ -121,6 +126,9 @@ __call_exitprocs (int code, void *d)
> > 	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
> > 	  else
> > 	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> > +#ifndef __SINGLE_THREAD__
> > +	  __lock_acquire_recursive(__atexit_recursive_mutex);
> > +#endif
> > 
> > 	  /* The function we called call atexit and registered another
> > 	     function (or functions).  Call these new functions before
> > --
> > 2.51.0
> 
> -- 
> embedded brains GmbH & Co. KG
> Herr Sebastian HUBER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: sebastian.huber@embedded-brains.de
> phone: +49-89-18 94 741 - 16
> fax:   +49-89-18 94 741 - 08
> 
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/
  

Hi Corinna, Sebasitian,

On Fri, 28 Nov 2025 13:21:35 +0100
Corinna Vinschen wrote:
> Hi Takashi, Hi Sebastian,
> 
> did you discuss this in PM, by any chance?  Is there any difference in
> GLibc or Darwin which explains how this potential problem is fixed
> there?

In glibc, the value of fnargs, etc is just copyied to local variable.
So, I think the followign patch is also OK for newlib.

diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
index 44f1f6acc..15ecda343 100644
--- a/newlib/libc/stdlib/__call_atexit.c
+++ b/newlib/libc/stdlib/__call_atexit.c
@@ -93,6 +93,8 @@ __call_exitprocs (int code, void *d)
       for (n = p->_ind - 1; n >= 0; n--)
 	{
 	  int ind;
+	  __ULong fntypes, is_cxa;
+	  void *fnarg;
 
 	  i = 1 << n;
 
@@ -114,18 +116,22 @@ __call_exitprocs (int code, void *d)
 
 	  ind = p->_ind;
 
+	  fntypes = args->_fntypes;
+	  is_cxa = args->_is_cxa;
+	  fnarg = args->_fnargs[n];
+
 #ifndef __SINGLE_THREAD__
 	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
 	     deadlock if it waits for another thread which calls atexit(). */
 	  __lock_release_recursive(__atexit_recursive_mutex);
 #endif
 	  /* Call the function.  */
-	  if (!args || (args->_fntypes & i) == 0)
+	  if (!args || (fntypes & i) == 0)
 	    fn ();
-	  else if ((args->_is_cxa & i) == 0)
-	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
+	  else if ((is_cxa & i) == 0)
+	    (*((void (*)(int, void *)) fn))(code, fnarg);
 	  else
-	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
+	    (*((void (*)(void *)) fn))(fnarg);
 #ifndef __SINGLE_THREAD__
 	  __lock_acquire_recursive(__atexit_recursive_mutex);
 #endif


> On Nov 26 05:17, Sebastian Huber wrote:
> > Hello Takashi Yano,
> > 
> > I have some questions to the change.
> > 
> > ----- Am 18. Nov 2025 um 14:31 schrieb Takashi Yano takashi.yano@nifty.ne.jp:
> > 
> > > The atexit()'ed function may deadlock if it waits for another thread
> > > which calls atexit() in the current __call_atexit.c code. This patch
> > > unlock __atexit_recursive_mutex while calling atexit()'ed functions
> > > to avoid the deadlock mentioned above. glibc and Darwin do the same,
> > > so it sounds reasonable.
> > > 
> > > Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
> > > Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
> > > Reviewed-by:
> > > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > > ---
> > > newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/newlib/libc/stdlib/__call_atexit.c
> > > b/newlib/libc/stdlib/__call_atexit.c
> > > index 710440389..44f1f6acc 100644
> > > --- a/newlib/libc/stdlib/__call_atexit.c
> > > +++ b/newlib/libc/stdlib/__call_atexit.c
> > > @@ -114,6 +114,11 @@ __call_exitprocs (int code, void *d)
> > > 
> > > 	  ind = p->_ind;
> > > 
> > > +#ifndef __SINGLE_THREAD__
> > > +	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
> > > +	     deadlock if it waits for another thread which calls atexit(). */
> > > +	  __lock_release_recursive(__atexit_recursive_mutex);
> > > +#endif
> > 
> > Here, a potentially shared structure is used through the args pointer. If some other thread calls exit() or atexit() concurrently, then this could result in a use of altered or freed memory. The data of the structure referenced by args should be first copied to local variables. Also I think that the deallocation should be done before we release the lock. We probably also have to restart the process after each handler call unconditionally.

Thanks for pointing this out.

What do you think of the patch above?

> > > 	  /* Call the function.  */
> > > 	  if (!args || (args->_fntypes & i) == 0)
> > > 	    fn ();
> > > @@ -121,6 +126,9 @@ __call_exitprocs (int code, void *d)
> > > 	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
> > > 	  else
> > > 	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> > > +#ifndef __SINGLE_THREAD__
> > > +	  __lock_acquire_recursive(__atexit_recursive_mutex);
> > > +#endif
> > > 
> > > 	  /* The function we called call atexit and registered another
> > > 	     function (or functions).  Call these new functions before
> > > --
> > > 2.51.0
  

Comparing with GLibc, this looks right to me.  Ok with you, Sebastian?


Thanks,
Corinna

On Nov 29 20:38, Takashi Yano wrote:
> Hi Corinna, Sebasitian,
> 
> On Fri, 28 Nov 2025 13:21:35 +0100
> Corinna Vinschen wrote:
> > Hi Takashi, Hi Sebastian,
> > 
> > did you discuss this in PM, by any chance?  Is there any difference in
> > GLibc or Darwin which explains how this potential problem is fixed
> > there?
> 
> In glibc, the value of fnargs, etc is just copyied to local variable.
> So, I think the followign patch is also OK for newlib.
> 
> diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
> index 44f1f6acc..15ecda343 100644
> --- a/newlib/libc/stdlib/__call_atexit.c
> +++ b/newlib/libc/stdlib/__call_atexit.c
> @@ -93,6 +93,8 @@ __call_exitprocs (int code, void *d)
>        for (n = p->_ind - 1; n >= 0; n--)
>  	{
>  	  int ind;
> +	  __ULong fntypes, is_cxa;
> +	  void *fnarg;
>  
>  	  i = 1 << n;
>  
> @@ -114,18 +116,22 @@ __call_exitprocs (int code, void *d)
>  
>  	  ind = p->_ind;
>  
> +	  fntypes = args->_fntypes;
> +	  is_cxa = args->_is_cxa;
> +	  fnarg = args->_fnargs[n];
> +
>  #ifndef __SINGLE_THREAD__
>  	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
>  	     deadlock if it waits for another thread which calls atexit(). */
>  	  __lock_release_recursive(__atexit_recursive_mutex);
>  #endif
>  	  /* Call the function.  */
> -	  if (!args || (args->_fntypes & i) == 0)
> +	  if (!args || (fntypes & i) == 0)
>  	    fn ();
> -	  else if ((args->_is_cxa & i) == 0)
> -	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
> +	  else if ((is_cxa & i) == 0)
> +	    (*((void (*)(int, void *)) fn))(code, fnarg);
>  	  else
> -	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> +	    (*((void (*)(void *)) fn))(fnarg);
>  #ifndef __SINGLE_THREAD__
>  	  __lock_acquire_recursive(__atexit_recursive_mutex);
>  #endif
> 
> 
> > On Nov 26 05:17, Sebastian Huber wrote:
> > > Hello Takashi Yano,
> > > 
> > > I have some questions to the change.
> > > 
> > > ----- Am 18. Nov 2025 um 14:31 schrieb Takashi Yano takashi.yano@nifty.ne.jp:
> > > 
> > > > The atexit()'ed function may deadlock if it waits for another thread
> > > > which calls atexit() in the current __call_atexit.c code. This patch
> > > > unlock __atexit_recursive_mutex while calling atexit()'ed functions
> > > > to avoid the deadlock mentioned above. glibc and Darwin do the same,
> > > > so it sounds reasonable.
> > > > 
> > > > Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html
> > > > Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp>
> > > > Reviewed-by:
> > > > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > > > ---
> > > > newlib/libc/stdlib/__call_atexit.c | 8 ++++++++
> > > > 1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/newlib/libc/stdlib/__call_atexit.c
> > > > b/newlib/libc/stdlib/__call_atexit.c
> > > > index 710440389..44f1f6acc 100644
> > > > --- a/newlib/libc/stdlib/__call_atexit.c
> > > > +++ b/newlib/libc/stdlib/__call_atexit.c
> > > > @@ -114,6 +114,11 @@ __call_exitprocs (int code, void *d)
> > > > 
> > > > 	  ind = p->_ind;
> > > > 
> > > > +#ifndef __SINGLE_THREAD__
> > > > +	  /* Unlock __atexit_recursive_mutex; otherwise, the function fn() may
> > > > +	     deadlock if it waits for another thread which calls atexit(). */
> > > > +	  __lock_release_recursive(__atexit_recursive_mutex);
> > > > +#endif
> > > 
> > > Here, a potentially shared structure is used through the args pointer. If some other thread calls exit() or atexit() concurrently, then this could result in a use of altered or freed memory. The data of the structure referenced by args should be first copied to local variables. Also I think that the deallocation should be done before we release the lock. We probably also have to restart the process after each handler call unconditionally.
> 
> Thanks for pointing this out.
> 
> What do you think of the patch above?
> 
> > > > 	  /* Call the function.  */
> > > > 	  if (!args || (args->_fntypes & i) == 0)
> > > > 	    fn ();
> > > > @@ -121,6 +126,9 @@ __call_exitprocs (int code, void *d)
> > > > 	    (*((void (*)(int, void *)) fn))(code, args->_fnargs[n]);
> > > > 	  else
> > > > 	    (*((void (*)(void *)) fn))(args->_fnargs[n]);
> > > > +#ifndef __SINGLE_THREAD__
> > > > +	  __lock_acquire_recursive(__atexit_recursive_mutex);
> > > > +#endif
> > > > 
> > > > 	  /* The function we called call atexit and registered another
> > > > 	     function (or functions).  Call these new functions before
> > > > --
> > > > 2.51.0
> 
> -- 
> Takashi Yano <takashi.yano@nifty.ne.jp>
  

Hello,

----- Am 1. Dez 2025 um 17:48 schrieb Corinna Vinschen vinschen@redhat.com:

> Comparing with GLibc, this looks right to me.  Ok with you, Sebastian?

yes, looks good, thanks.

There could be still some use after free issues, however, I would have to develop a test case for this to be sure.