Re: Bug in __register_exitproc() in __atexit.c

[Freddie Chopin <freddie_chopin at op dot pl>]

On Wednesday 16 of December 2015 17:25:23 Jeff Johnston wrote:
> Use the same tactic that is used in __atexit.c.
> 
> Declare a static structure in a new .c file and create a const pointer to it
> called __on_exit_args.  (see libc/reent/impure.c for an example of how this
> is done for the reent struct).
> 
> In __register_exitproc, declare it as weak and refer to it when needed due
> to the flags.
> 
> In on_exit and __cxa_exit, use the flags to determine when to declare a
> dummy global variable such that you have:
> 
> #if defined(FLAGS_NEEDED...)
> const void *__some_dummy_var = &__on_exit_args;
> #endif
> 
> Applications that link in on_exit or __cxa_exit will drag in the struct if
> configured with the flags.  Other applications will not get the added bloat
> of the struct.
> 
> I have a simple example library if it is still unclear.

OK, I've done it as described by Jeff and it works (; When only atexit() is 
used, then the static instance of _on_exit_args struct is not linked. When 
either on_exit() or __cxa_atexit() are used, then the static instance is 
linked into the executable.

The static instance of _on_exit_args struct is used when "small reent" 
configuration is used. In theory it could be limited to "reent small" and "no 
dynamic allocation in atexit()", but the approach I took seemed more 
appropriate. on_exit()'s description in newlib's docs says that there are 32 
statically allocated slots, so with this fix this is really true for all 
configurations.

I attach two patches that implement relevant changes - the first one actually 
adds the static instance of _on_exit_args struct, while the second one is a 
fix for malloc() usage in __register_exitproc(). The second patch is mostly 
identical to what I sent previously, but with changed commit message (there's 
no problem with _on_exit_args after the first patch is applied).

I've verified that all 8 combinations of relevant options (reent small, 
dynamic alloc in atexit, global atexit) can be compiled.

Regards,
FCh

>From 8b8776de46550234c395e5bd215cdb92e95ee21f Mon Sep 17 00:00:00 2001
From: Freddie Chopin <freddie.chopin@gmail.com>
Date: Sun, 20 Dec 2015 00:24:40 +0100
Subject: [PATCH 1/2] Add static instance of _on_exit_args for _REENT_SMALL
 configuration

When _REENT_SMALL is used, _atexit struct only contains a pointer to
_on_exit_args struct, so this was always allocated with malloc() - even
for the first 32 calls of atexit()-like functions, which are guaranteed
to succeed, but could fail because of "out of memory" error.

Thats why a static instance of _on_exit_args struct is provided for
_REENT_SMALL configuration. This way the first 32 calls to atexit()-like
functions don't need malloc() and will always succeed.

Because this struct is not needed for "normal" atexit(), it is used as a
weak reference in __register_exitproc(), but any use of on_exit() or
__cxa_atexit() will force it to be linked.

        * libc/stdlib/on_exit_args.{c,h}: New files.
        * libc/stdlib/Makefile.am: Add new source file.
        * libc/stdlib/Makefile.in: Regenerate.
        * libc/stdlib/__atexit.c (__register_exitproc): Initialize
        _on_exit_args_ptr field of _GLOBAL_ATEXIT on first run.
        * libc/stdlib/on_exit.c: Force linking of static instance of
        _on_exit_args.
        * libc/stdlib/cxa_atexit.c: Likewise.
---
 newlib/libc/stdlib/Makefile.am    |  1 +
 newlib/libc/stdlib/Makefile.in    | 48 ++++++++++++++++++++++-----------------
 newlib/libc/stdlib/__atexit.c     |  9 +++++++-
 newlib/libc/stdlib/cxa_atexit.c   |  9 ++++++++
 newlib/libc/stdlib/on_exit.c      |  9 ++++++++
 newlib/libc/stdlib/on_exit_args.c | 30 ++++++++++++++++++++++++
 newlib/libc/stdlib/on_exit_args.h | 12 ++++++++++
 7 files changed, 96 insertions(+), 22 deletions(-)
 create mode 100644 newlib/libc/stdlib/on_exit_args.c
 create mode 100644 newlib/libc/stdlib/on_exit_args.h

diff --git a/newlib/libc/stdlib/Makefile.am b/newlib/libc/stdlib/Makefile.am
index 33d4a0c..c1067ea 100644
--- a/newlib/libc/stdlib/Makefile.am
+++ b/newlib/libc/stdlib/Makefile.am
@@ -46,6 +46,7 @@ GENERAL_SOURCES = \
 	mlock.c		\
 	mprec.c		\
 	mstats.c	\
+	on_exit_args.c	\
 	quick_exit.c	\
 	rand.c		\
 	rand_r.c	\
diff --git a/newlib/libc/stdlib/Makefile.in b/newlib/libc/stdlib/Makefile.in
index d0368c0..36e81c8 100644
--- a/newlib/libc/stdlib/Makefile.in
+++ b/newlib/libc/stdlib/Makefile.in
@@ -101,17 +101,17 @@ am__objects_2 = lib_a-__adjust.$(OBJEXT) lib_a-__atexit.$(OBJEXT) \
 	lib_a-mbstowcs.$(OBJEXT) lib_a-mbstowcs_r.$(OBJEXT) \
 	lib_a-mbtowc.$(OBJEXT) lib_a-mbtowc_r.$(OBJEXT) \
 	lib_a-mlock.$(OBJEXT) lib_a-mprec.$(OBJEXT) \
-	lib_a-mstats.$(OBJEXT) lib_a-quick_exit.$(OBJEXT) \
-	lib_a-rand.$(OBJEXT) lib_a-rand_r.$(OBJEXT) \
-	lib_a-realloc.$(OBJEXT) lib_a-reallocf.$(OBJEXT) \
-	lib_a-sb_charsets.$(OBJEXT) lib_a-strtod.$(OBJEXT) \
-	lib_a-strtodg.$(OBJEXT) lib_a-strtol.$(OBJEXT) \
-	lib_a-strtorx.$(OBJEXT) lib_a-strtoul.$(OBJEXT) \
-	lib_a-utoa.$(OBJEXT) lib_a-wcstod.$(OBJEXT) \
-	lib_a-wcstol.$(OBJEXT) lib_a-wcstoul.$(OBJEXT) \
-	lib_a-wcstombs.$(OBJEXT) lib_a-wcstombs_r.$(OBJEXT) \
-	lib_a-wctomb.$(OBJEXT) lib_a-wctomb_r.$(OBJEXT) \
-	$(am__objects_1)
+	lib_a-mstats.$(OBJEXT) lib_a-on_exit_args.$(OBJEXT) \
+	lib_a-quick_exit.$(OBJEXT) lib_a-rand.$(OBJEXT) \
+	lib_a-rand_r.$(OBJEXT) lib_a-realloc.$(OBJEXT) \
+	lib_a-reallocf.$(OBJEXT) lib_a-sb_charsets.$(OBJEXT) \
+	lib_a-strtod.$(OBJEXT) lib_a-strtodg.$(OBJEXT) \
+	lib_a-strtol.$(OBJEXT) lib_a-strtorx.$(OBJEXT) \
+	lib_a-strtoul.$(OBJEXT) lib_a-utoa.$(OBJEXT) \
+	lib_a-wcstod.$(OBJEXT) lib_a-wcstol.$(OBJEXT) \
+	lib_a-wcstoul.$(OBJEXT) lib_a-wcstombs.$(OBJEXT) \
+	lib_a-wcstombs_r.$(OBJEXT) lib_a-wctomb.$(OBJEXT) \
+	lib_a-wctomb_r.$(OBJEXT) $(am__objects_1)
 am__objects_3 = lib_a-cxa_atexit.$(OBJEXT) \
 	lib_a-cxa_finalize.$(OBJEXT) lib_a-drand48.$(OBJEXT) \
 	lib_a-ecvtbuf.$(OBJEXT) lib_a-efgcvt.$(OBJEXT) \
@@ -157,11 +157,11 @@ am__objects_9 = __adjust.lo __atexit.lo __call_atexit.lo __exp10.lo \
 	exit.lo gdtoa-gethex.lo gdtoa-hexnan.lo getenv.lo getenv_r.lo \
 	itoa.lo labs.lo ldiv.lo ldtoa.lo malloc.lo mblen.lo mblen_r.lo \
 	mbstowcs.lo mbstowcs_r.lo mbtowc.lo mbtowc_r.lo mlock.lo \
-	mprec.lo mstats.lo quick_exit.lo rand.lo rand_r.lo realloc.lo \
-	reallocf.lo sb_charsets.lo strtod.lo strtodg.lo strtol.lo \
-	strtorx.lo strtoul.lo utoa.lo wcstod.lo wcstol.lo wcstoul.lo \
-	wcstombs.lo wcstombs_r.lo wctomb.lo wctomb_r.lo \
-	$(am__objects_8)
+	mprec.lo mstats.lo on_exit_args.lo quick_exit.lo rand.lo \
+	rand_r.lo realloc.lo reallocf.lo sb_charsets.lo strtod.lo \
+	strtodg.lo strtol.lo strtorx.lo strtoul.lo utoa.lo wcstod.lo \
+	wcstol.lo wcstoul.lo wcstombs.lo wcstombs_r.lo wctomb.lo \
+	wctomb_r.lo $(am__objects_8)
 am__objects_10 = cxa_atexit.lo cxa_finalize.lo drand48.lo ecvtbuf.lo \
 	efgcvt.lo erand48.lo jrand48.lo lcong48.lo lrand48.lo \
 	mrand48.lo msize.lo mtrim.lo nrand48.lo rand48.lo seed48.lo \
@@ -361,11 +361,11 @@ GENERAL_SOURCES = __adjust.c __atexit.c __call_atexit.c __exp10.c \
 	dtoastub.c environ.c envlock.c eprintf.c exit.c gdtoa-gethex.c \
 	gdtoa-hexnan.c getenv.c getenv_r.c itoa.c labs.c ldiv.c \
 	ldtoa.c malloc.c mblen.c mblen_r.c mbstowcs.c mbstowcs_r.c \
-	mbtowc.c mbtowc_r.c mlock.c mprec.c mstats.c quick_exit.c \
-	rand.c rand_r.c realloc.c reallocf.c sb_charsets.c strtod.c \
-	strtodg.c strtol.c strtorx.c strtoul.c utoa.c wcstod.c \
-	wcstol.c wcstoul.c wcstombs.c wcstombs_r.c wctomb.c wctomb_r.c \
-	$(am__append_1)
+	mbtowc.c mbtowc_r.c mlock.c mprec.c mstats.c on_exit_args.c \
+	quick_exit.c rand.c rand_r.c realloc.c reallocf.c \
+	sb_charsets.c strtod.c strtodg.c strtol.c strtorx.c strtoul.c \
+	utoa.c wcstod.c wcstol.c wcstoul.c wcstombs.c wcstombs_r.c \
+	wctomb.c wctomb_r.c $(am__append_1)
 @NEWLIB_NANO_MALLOC_FALSE@MALIGNR = malignr
 @NEWLIB_NANO_MALLOC_TRUE@MALIGNR = nano-malignr
 @NEWLIB_NANO_MALLOC_FALSE@MALLOPTR = malloptr
@@ -856,6 +856,12 @@ lib_a-mstats.o: mstats.c
 lib_a-mstats.obj: mstats.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-mstats.obj `if test -f 'mstats.c'; then $(CYGPATH_W) 'mstats.c'; else $(CYGPATH_W) '$(srcdir)/mstats.c'; fi`
 
+lib_a-on_exit_args.o: on_exit_args.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-on_exit_args.o `test -f 'on_exit_args.c' || echo '$(srcdir)/'`on_exit_args.c
+
+lib_a-on_exit_args.obj: on_exit_args.c
+	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-on_exit_args.obj `if test -f 'on_exit_args.c'; then $(CYGPATH_W) 'on_exit_args.c'; else $(CYGPATH_W) '$(srcdir)/on_exit_args.c'; fi`
+
 lib_a-quick_exit.o: quick_exit.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-quick_exit.o `test -f 'quick_exit.c' || echo '$(srcdir)/'`quick_exit.c
 
diff --git a/newlib/libc/stdlib/__atexit.c b/newlib/libc/stdlib/__atexit.c
index a3d5bdf..23eab29 100644
--- a/newlib/libc/stdlib/__atexit.c
+++ b/newlib/libc/stdlib/__atexit.c
@@ -79,7 +79,14 @@ _DEFUN (__register_exitproc,
 
   p = _GLOBAL_ATEXIT;
   if (p == NULL)
-    _GLOBAL_ATEXIT = p = _GLOBAL_ATEXIT0;
+    {
+      _GLOBAL_ATEXIT = p = _GLOBAL_ATEXIT0;
+#ifdef _REENT_SMALL
+      extern struct _on_exit_args * const __on_exit_args _ATTRIBUTE ((weak));
+      if (&__on_exit_args != NULL)
+	p->_on_exit_args_ptr = __on_exit_args;
+#endif	/* def _REENT_SMALL */
+    }
   if (p->_ind >= _ATEXIT_SIZE)
     {
 #ifndef _ATEXIT_DYNAMIC_ALLOC
diff --git a/newlib/libc/stdlib/cxa_atexit.c b/newlib/libc/stdlib/cxa_atexit.c
index c3c0d2a..39a59d5 100644
--- a/newlib/libc/stdlib/cxa_atexit.c
+++ b/newlib/libc/stdlib/cxa_atexit.c
@@ -8,6 +8,15 @@
 #include <sys/lock.h>
 #include "atexit.h"
 
+#ifdef _REENT_SMALL
+
+#include "on_exit_args.h"
+
+/* force linking of static instance of _on_exit_args */
+const void * const __cxa_atexit_dummy = &__on_exit_args;
+
+#endif	/* def _REENT_SMALL */
+
 /*
  * Register a function to be performed at exit or DSO unload.
  */
diff --git a/newlib/libc/stdlib/on_exit.c b/newlib/libc/stdlib/on_exit.c
index b7fd130..a405b1b 100644
--- a/newlib/libc/stdlib/on_exit.c
+++ b/newlib/libc/stdlib/on_exit.c
@@ -58,6 +58,15 @@ Supporting OS subroutines required: None
 #include <stdlib.h>
 #include "atexit.h"
 
+#ifdef _REENT_SMALL
+
+#include "on_exit_args.h"
+
+/* force linking of static instance of _on_exit_args */
+const void * const __on_exit_dummy = &__on_exit_args;
+
+#endif	/* def _REENT_SMALL */
+
 /*
  * Register a function to be performed at exit.
  */
diff --git a/newlib/libc/stdlib/on_exit_args.c b/newlib/libc/stdlib/on_exit_args.c
new file mode 100644
index 0000000..88f9ffd
--- /dev/null
+++ b/newlib/libc/stdlib/on_exit_args.c
@@ -0,0 +1,30 @@
+/*
+ * Static instance of _on_exit_args struct.
+ *
+ * When _REENT_SMALL is used, _atexit struct only contains a pointer to
+ * _on_exit_args struct, so this was always allocated with malloc() - even for
+ * the first 32 calls of atexit()-like functions, which are guaranteed to
+ * succeed, but could fail because of "out of memory" error. This is even worse
+ * when _ATEXIT_DYNAMIC_ALLOC is _NOT_ defined, in which case malloc() is not
+ * used by internals of atexit()-like functions. In such configuration all calls
+ * to the functions that need _on_exit_args struct (on_exit() and
+ * __cxa_atexit()) would fail.
+ *
+ * Thats why a static instance of _on_exit_args struct is provided for
+ * _REENT_SMALL configuration. This way the first 32 calls to atexit()-like
+ * functions don't need malloc() and will always succeed.
+ *
+ * Because this struct is not needed for "normal" atexit(), it is used as a weak
+ * reference in __register_exitproc(), but any use of on_exit() or
+ * __cxa_atexit() will force it to be linked.
+ */
+
+#include <reent.h>
+
+#ifdef _REENT_SMALL
+
+static struct _on_exit_args _on_exit_args_instance = {{_NULL}, {_NULL}, 0, 0};
+
+struct _on_exit_args * const __on_exit_args = &_on_exit_args_instance;
+
+#endif	/* def _REENT_SMALL */
diff --git a/newlib/libc/stdlib/on_exit_args.h b/newlib/libc/stdlib/on_exit_args.h
new file mode 100644
index 0000000..c54ad1e
--- /dev/null
+++ b/newlib/libc/stdlib/on_exit_args.h
@@ -0,0 +1,12 @@
+#ifndef NEWLIB_CYGWIN_NEWLIB_LIBC_STDLIB_ON_EXIT_ARGS_H_
+#define NEWLIB_CYGWIN_NEWLIB_LIBC_STDLIB_ON_EXIT_ARGS_H_
+
+#include <reent.h>
+
+#ifdef _REENT_SMALL
+
+extern struct _on_exit_args * const __on_exit_args;
+
+#endif	/* def _REENT_SMALL */
+
+#endif	/* def NEWLIB_CYGWIN_NEWLIB_LIBC_STDLIB_ON_EXIT_ARGS_H_ */
-- 
2.6.4

>From 348bdf2b1a3f5a6bb3decbb28e1eb8972101bcfb Mon Sep 17 00:00:00 2001
From: Freddie Chopin <freddie.chopin@gmail.com>
Date: Sun, 20 Dec 2015 00:25:50 +0100
Subject: [PATCH 2/2] Fix for _ATEXIT_DYNAMIC_ALLOC in __register_exitproc()

If small reent is enabled (_REENT_SMALL is defined) then malloc() was
used in __register_exitproc() even if user requested it to be disabled
(_ATEXIT_DYNAMIC_ALLOC is defined). With this fix, function fails when
_ATEXIT_DYNAMIC_ALLOC is defined and whole static storage is already
used.

        * libc/stdlib/__atexit.c (__register_exitproc): Fix for
        _ATEXIT_DYNAMIC_ALLOC.
---
 newlib/libc/stdlib/__atexit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/newlib/libc/stdlib/__atexit.c b/newlib/libc/stdlib/__atexit.c
index 23eab29..d07f6c1 100644
--- a/newlib/libc/stdlib/__atexit.c
+++ b/newlib/libc/stdlib/__atexit.c
@@ -131,6 +131,12 @@ _DEFUN (__register_exitproc,
       args = p->_on_exit_args_ptr;
       if (args == NULL)
 	{
+#ifndef _ATEXIT_DYNAMIC_ALLOC
+#ifndef __SINGLE_THREAD__
+	  __lock_release_recursive(__atexit_lock);
+#endif
+	  return -1;
+#else
 	  if (malloc)
 	    args = malloc (sizeof * p->_on_exit_args_ptr);
 
@@ -144,6 +150,7 @@ _DEFUN (__register_exitproc,
 	  args->_fntypes = 0;
 	  args->_is_cxa = 0;
 	  p->_on_exit_args_ptr = args;
+#endif
 	}
 #else
       args = &p->_on_exit_args;
-- 
2.6.4