Re: [PATCH] newlib: Unlock the mutex while calling atexit()'ed functions

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 [email protected]:
> > 
> > > 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 <[email protected]>
> > > Reviewed-by:
> > > Signed-off-by: Takashi Yano <[email protected]>
> > > ---
> > > 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 <[email protected]>