Re: Bug in __register_exitproc() in __atexit.c

[Jeff Johnston <jjohnstn at redhat dot com>]

----- Original Message -----
> On Dec 12 15:25, Freddie Chopin wrote:
> > On Wednesday 09 of December 2015 10:27:31 Corinna Vinschen wrote:
> > > On Dec  8 16:40, Freddie Chopin wrote:
> > > > Hello!
> > > > 
> > > > Today I noticed that even when I disable dynamic allocation of memory
> > > > in
> > > > atexit (with --disable-newlib-atexit-dynamic-alloc switch) malloc() is
> > > > used in __register_exitproc().
> > > > 
> > > > First of all, if this switch is used, then _ATEXIT_DYNAMIC_ALLOC is not
> > > > defined, so the first use of malloc() in this function - at line 93 -
> > > > is
> > > > indeed disabled. But if small reent is used, malloc() is used again at
> > > > line
> > > > 120 - this time it's not conditional on _ATEXIT_DYNAMIC_ALLOC being
> > > > defined.
> > > > 
> > > > Moreover, at first glance it seems that there's a bug in this function.
> > > > The
> > > > __atexit_lock lock is acquired at entry, but there are at two places
> > > > where
> > > > it is not released before return - at line 86 and at line 91.
> > > > 
> > > > Should I try to provide a patch for both issues?
> > > 
> > > That would be nice :)
> > 
> > OK, I've just sent the patches.
> > 
> > I've checked that newlib still compiles with all 8 combinations of related
> > options (reent small, global atexit, dynamic atexit). The fix I implemented
> > also solves the issue I had - __register_exitproc() no longer uses malloc()
> > if
> > --disable-newlib-atexit-dynamic-alloc was used.
> > 
> > The first patch - the one for missing lock releases - is trivial and
> > there's
> > not much to debate.
> > 
> > The second one - with the fix for dynamic atexit allocation - is more
> > problematic... It turns out that when both --disable-newlib-atexit-dynamic-
> > alloc and --enable-newlib-reent-small are used, then __register_exitproc()
> > called by on_exit() or __cxa_atexit() will never succeed. Both of these
> > variants pass an argument to the registered function and this argument is
> > placed in a struct that is _NOT_ part of _atexit struct.
> > 
> > This can actually be solved easily - you'd just need to allocate a static
> > instance of _on_exit_args struct and use it for the first 32 registered
> > functions. The only problem is that the size of this struct is - for 32-bit
> > ARM - 264 bytes, so not exactly "small". So I was wondering what approach
> > would you suggest? Leave it as it is (with the fix) or maybe add static
> > instance of this struct when dynamic allocation is disabled for atexit-like
> > functions?
> > 
> > Maybe it would be possible to have the instance of this struct as a weak
> > reference and pull it in only if on_exit() or __cxa_atexit() are used?
> > 
> > Regards,
> > FCh
> 
> I'd like to defer this to Jeff.  I'm not sure what's the right or best
> way to fix this.
> 
> Jeff?
>

I think I like the weak reference idea.  Have register_exitproc make a weak reference
and have on_exit and __cxa_atexit make a strong reference.  If someone uses the configuration
flags "and" uses on_exit or __cxa_atexit, then they pull in the additional static
storage and that is the cost of using them.

-- Jeff J.
 
> 
> Thanks,
> Corinna
> 
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>