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

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 [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]>