[PATCH] fix atexit when it is called from an atexit handler

[PATCH] fix atexit when it is called from an atexit handler

[Szabolcs Nagy]

When running atexit handlers always restart iterating the list of
handlers so if new ones are registered they are called in the right
order.

Note that the iteration changes the head pointer in an irreversible
way, this is not a problem, but may cause more allocations than
optimal when new handlers are registered during exit.

The old code accepted atexit handlers after exit, but did not run
them.  C11 seems to explicitly allow atexit to fail in this case,
but this situation can easily come up in C++ if a destructor has
local static object with a destructor so it should be handled.
---
 src/exit/atexit.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/exit/atexit.c b/src/exit/atexit.c
index be82718..a0fd8d7 100644
--- a/src/exit/atexit.c
+++ b/src/exit/atexit.c
@@ -14,13 +14,24 @@ static struct fl
 
 static volatile int lock[2];
 
+static int next()
+{
+	int i;
+	for (; head; head=head->next)
+		for (i=COUNT-1; i>=0; i--)
+			if (head->f[i])
+				return i;
+	return -1;
+}
+
 void __funcs_on_exit()
 {
 	int i;
 	void (*func)(void *), *arg;
 	LOCK(lock);
-	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
-		if (!head->f[i]) continue;
+	for (;;) {
+		i = next();
+		if (i<0) break;
 		func = head->f[i];
 		arg = head->a[i];
 		head->f[i] = 0;
-- 
2.4.1

Re: [PATCH] fix atexit when it is called from an atexit handler

[Rich Felker]

On Thu, Jul 23, 2015 at 01:44:06AM +0200, Szabolcs Nagy wrote:
> When running atexit handlers always restart iterating the list of
> handlers so if new ones are registered they are called in the right
> order.
> 
> Note that the iteration changes the head pointer in an irreversible
> way, this is not a problem, but may cause more allocations than
> optimal when new handlers are registered during exit.
> 
> The old code accepted atexit handlers after exit, but did not run
> them.  C11 seems to explicitly allow atexit to fail in this case,
> but this situation can easily come up in C++ if a destructor has
> local static object with a destructor so it should be handled.
> ---
>  src/exit/atexit.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/exit/atexit.c b/src/exit/atexit.c
> index be82718..a0fd8d7 100644
> --- a/src/exit/atexit.c
> +++ b/src/exit/atexit.c
> @@ -14,13 +14,24 @@ static struct fl
>  
>  static volatile int lock[2];
>  
> +static int next()
> +{
> +	int i;
> +	for (; head; head=head->next)
> +		for (i=COUNT-1; i>=0; i--)
> +			if (head->f[i])
> +				return i;
> +	return -1;
> +}
> +
>  void __funcs_on_exit()
>  {
>  	int i;
>  	void (*func)(void *), *arg;
>  	LOCK(lock);
> -	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
> -		if (!head->f[i]) continue;
> +	for (;;) {
> +		i = next();
> +		if (i<0) break;
>  		func = head->f[i];
>  		arg = head->a[i];
>  		head->f[i] = 0;

I agree that this change should be made, but I don't like the
particulars of the patch. It seems to increase exit handler runtime to
O(n²) even in the case where no atexit is happening during exit, and
it's not very elegant. I think a simpler solution would be to reset i
to COUNT after calling f[i] if either (1) head has changed, or (2)
i<COUNT-1 and f[i+1] is nonzero. Does this sound correct/better?

Rich

Re: [PATCH] fix atexit when it is called from an atexit handler

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

* Rich Felker <dalias@libc.org> [2015-07-22 21:18:16 -0400]:
> On Thu, Jul 23, 2015 at 01:44:06AM +0200, Szabolcs Nagy wrote:
> >  void __funcs_on_exit()
> >  {
> >  	int i;
> >  	void (*func)(void *), *arg;
> >  	LOCK(lock);
> > -	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
> > -		if (!head->f[i]) continue;
> > +	for (;;) {
> > +		i = next();
> > +		if (i<0) break;
> >  		func = head->f[i];
> >  		arg = head->a[i];
> >  		head->f[i] = 0;
> 
> I agree that this change should be made, but I don't like the
> particulars of the patch. It seems to increase exit handler runtime to
> O(n²) even in the case where no atexit is happening during exit, and
> it's not very elegant. I think a simpler solution would be to reset i
> to COUNT after calling f[i] if either (1) head has changed, or (2)
> i<COUNT-1 and f[i+1] is nonzero. Does this sound correct/better?

(2) should be 'f[i] is nonzero'.

i didnt think about detecting the change of head,
but it is simpler and better for the common case.

[-- Attachment #2: 0001-fix-atexit-when-it-is-called-from-an-atexit-handler.patch --]
[-- Type: text/x-diff, Size: 1452 bytes --]

From 63c2b2d58b2ec45df67927b8959ea8af6dd578ed Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Wed, 22 Jul 2015 23:05:57 +0000
Subject: [PATCH] fix atexit when it is called from an atexit handler

The old code accepted atexit handlers after exit, but did not run
them.  C11 seems to explicitly allow atexit to fail in this case,
but this situation can easily come up in C++ if a destructor has
local static object with a destructor so it should be handled.

Restart iterating the list of atexit handlers if new handlers are
added during a call.

Note that the memory usage can grow linearly with the overall
number of registered atexit handlers instead of with the worst
case list length. (This only matters if atexit handlers keep
registering atexit handlers which should not happen in practice).
---
 src/exit/atexit.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/exit/atexit.c b/src/exit/atexit.c
index be82718..75f7511 100644
--- a/src/exit/atexit.c
+++ b/src/exit/atexit.c
@@ -17,6 +17,7 @@ static volatile int lock[2];
 void __funcs_on_exit()
 {
 	int i;
+	struct fl *old;
 	void (*func)(void *), *arg;
 	LOCK(lock);
 	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
@@ -24,9 +25,12 @@ void __funcs_on_exit()
 		func = head->f[i];
 		arg = head->a[i];
 		head->f[i] = 0;
+		old = head;
 		UNLOCK(lock);
 		func(arg);
 		LOCK(lock);
+		if (head != old || head->f[i])
+			i = COUNT;
 	}
 }
 
-- 
2.4.1

Re: [PATCH] fix atexit when it is called from an atexit handler

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

Am Donnerstag, den 23.07.2015, 05:07 +0200 schrieb Szabolcs Nagy:
> * Rich Felker <dalias@libc.org> [2015-07-22 21:18:16 -0400]:
> > On Thu, Jul 23, 2015 at 01:44:06AM +0200, Szabolcs Nagy wrote:
> > >  void __funcs_on_exit()
> > >  {
> > >  	int i;
> > >  	void (*func)(void *), *arg;
> > >  	LOCK(lock);
> > > -	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
> > > -		if (!head->f[i]) continue;
> > > +	for (;;) {
> > > +		i = next();
> > > +		if (i<0) break;
> > >  		func = head->f[i];
> > >  		arg = head->a[i];
> > >  		head->f[i] = 0;
> > 
> > I agree that this change should be made, but I don't like the
> > particulars of the patch. It seems to increase exit handler runtime to
> > O(n²) even in the case where no atexit is happening during exit, and
> > it's not very elegant. I think a simpler solution would be to reset i
> > to COUNT after calling f[i] if either (1) head has changed, or (2)
> > i<COUNT-1 and f[i+1] is nonzero. Does this sound correct/better?
> 
> (2) should be 'f[i] is nonzero'.
> 
> i didnt think about detecting the change of head,
> but it is simpler and better for the common case.

I'd think this could be done even simpler by running the handlers in
batches. Something like

 (a) take the lock and save the head of the current list in a tmp
 variable
 (b) release the lock
 (c) iterate through the list that was saved in tmp
 (d) if now head is non-zero, goto (a)

This is clearly linear in the number of handlers that are ever insert
to the list.

For the current implementation this would need a bit of change in
__cxa_atexit because 0 for head wouldn't mean that it hasn't been
used, yet.

A similar improvement could be done for at_quick_exit. There I think
it makes not much sense accepting new handlers during processing. So
the strategy could just be

  (i) Take the lock and save count in a tmp
  (ii) set count to 32
  (iii) unlock
  (iv) do the processing

But looking at it, I think that one can even be more improved. We
could just use count as atomic, and so we wouldn't need a lock.
I can prepare a patch for that, if you like.

Jens


-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] fix atexit when it is called from an atexit handler

[Jens Gustedt]

[-- Attachment #1.1: Type: text/plain, Size: 1643 bytes --]

Am Donnerstag, den 23.07.2015, 07:54 +0200 schrieb Jens Gustedt:
> I'd think this could be done even simpler by running the handlers in
> batches. ...

I checked, the strict ordering constraints for the handlers don't
allow for such a strategy, so forget it.

Looking at the code, I am a bit uncomfortable with the idea of a
potentially unlimited number of calloc calls during exit. I think it
would be good if we could restrict the maximal number of successful
calls to atexit during exit to 32. A calloc-free strategy could be to
save head to a tmp a the beginning of processing and to provide a
`struct fl` table on the stack of __funcs_on_exit.

> A similar improvement could be done for at_quick_exit. There I think
> it makes not much sense accepting new handlers during processing. So
> the strategy could just be
> 
>   (i) Take the lock and save count in a tmp
>   (ii) set count to 32
>   (iii) unlock
>   (iv) do the processing
> 
> But looking at it, I think that one can even be more improved. We
> could just use count as atomic, and so we wouldn't need a lock.
> I can prepare a patch for that, if you like.

Since this is fairly simple, I just did it. Please find a new version
attached. (too much changes, a patch makes no sense.)  This compiles,
but I have no code to test this.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #1.2: at_quick_exit.c --]
[-- Type: text/x-csrc, Size: 679 bytes --]

#include <stdlib.h>
#include "atomic.h"
#include "libc.h"

#define COUNT 32

static void (*funcs[COUNT])(void);
static volatile int count;

void __funcs_on_quick_exit()
{
	void (*func)(void);
	/* No need for atomics here, we are alone. */
	int total = count;
	/* The counter might have been overrun at some time. */
	if (total > 32) total = 32;
 	else count = 32;
	while (total > 0) {
		func = funcs[--total];
		func();
	}
}

int at_quick_exit(void (*func)(void))
{
	int pos = a_fetch_add(&count, 1);
	if (pos >= 32) {
		if (pos > INT_MAX/2) pos = 32;
		return -1;
        }
	funcs[pos] = func;
	/* make sure that the effect is visible to everybody */
	a_barrier();
	return 0;
}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] fix atexit when it is called from an atexit handler

[Rich Felker]

On Thu, Jul 23, 2015 at 09:19:13AM +0200, Jens Gustedt wrote:
> Am Donnerstag, den 23.07.2015, 07:54 +0200 schrieb Jens Gustedt:
> > I'd think this could be done even simpler by running the handlers in
> > batches. ...
> 
> I checked, the strict ordering constraints for the handlers don't
> allow for such a strategy, so forget it.

Indeed, this is not possible.

> Looking at the code, I am a bit uncomfortable with the idea of a
> potentially unlimited number of calloc calls during exit. I think it

Is there a specific reason? This will only happen in pathological code
that keeps registering atexit handlers, and eventually the allocations
will fail and therefore atexit will fail, which it's allowed to do.
The only thing pathological that happens is that memory usage keeps
increasing even if you've balanced the running and registering of
atexit handlers so that the total number registered always stays
bounded, but we're still talking about extreme abuse of the atexit
API, and I think optimizing this case without an argument that it's
worthwhile would probably be premature optimization.

> would be good if we could restrict the maximal number of successful
> calls to atexit during exit to 32. A calloc-free strategy could be to
> save head to a tmp a the beginning of processing and to provide a
> `struct fl` table on the stack of __funcs_on_exit.

I'm not sure how this would be better. It would be more predictable,
but could also probably break some excessive but "valid" use (like
a huge chain of ctors getting called from an atexit handler and all
registering dtors).

> > A similar improvement could be done for at_quick_exit. There I think
> > it makes not much sense accepting new handlers during processing. So
> > the strategy could just be
> > 
> >   (i) Take the lock and save count in a tmp
> >   (ii) set count to 32
> >   (iii) unlock
> >   (iv) do the processing
> > 
> > But looking at it, I think that one can even be more improved. We
> > could just use count as atomic, and so we wouldn't need a lock.
> > I can prepare a patch for that, if you like.
> 
> Since this is fairly simple, I just did it. Please find a new version
> attached. (too much changes, a patch makes no sense.)  This compiles,
> but I have no code to test this.

The current program in musl is not to treat replacements of locks by
an atomics as an improvement, but rather the opposite. Atomics should
not be used unless they're needed for semantic correctness (e.g. in
situations where AS-safety is required and could not be obtained with
locks, or at least not without expensive signal masking if locks were
to be used) or a compelling performance reason, and atexit is surely
not a performance bottleneck. See these recent commits that have
removed or limited the use of atomics:

63c188ec42e76ff768e81f6b65b11c68fc43351e replace atomics with locks in locale-setting code
68630b55c0c7219fe9df70dc28ffbf9efc8021d8 eliminate costly tricks to avoid TLS access for current locale state
6de071a0be00ec2ff08af3c89c7caaa20f1044d7 eliminate atomics in syslog setlogmask function
fd850de7524d8b62cd1d340417b665ed9427adae fix type error (arch-dependent) in new aio code
4e8a3561652ebcda6a126b3162fc545573889dc4 overhaul aio implementation for correctness

The reasoning behind this approach is that direct use of atomics is a
high entry barrier to understanding the code, a major risk of bugs
from subtle errors, and precludes the code using atomics from
benefitting from potential future improvements to synchronization
primitives (like weak memory order, lock elision, etc.).

Certainly the implementations of synchronization primitives
themselves, and some other low-level code (like dynamic TLS setup)
must keep using atomics, and future malloc improvements may benefit
from direct atomic usage, but I'd like to get rid of most/all other
uses.

Rich

Re: [PATCH] fix atexit when it is called from an atexit handler

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

Am Donnerstag, den 23.07.2015, 15:58 -0400 schrieb Rich Felker:
> On Thu, Jul 23, 2015 at 09:19:13AM +0200, Jens Gustedt wrote:
> > Am Donnerstag, den 23.07.2015, 07:54 +0200 schrieb Jens Gustedt:
> > > I'd think this could be done even simpler by running the handlers in
> > > batches. ...
> > 
> > I checked, the strict ordering constraints for the handlers don't
> > allow for such a strategy, so forget it.
> 
> Indeed, this is not possible.
> 
> > Looking at the code, I am a bit uncomfortable with the idea of a
> > potentially unlimited number of calloc calls during exit. I think it
> 
> Is there a specific reason? This will only happen in pathological code
> that keeps registering atexit handlers, and eventually the allocations
> will fail and therefore atexit will fail, which it's allowed to do.
> The only thing pathological that happens is that memory usage keeps
> increasing even if you've balanced the running and registering of
> atexit handlers so that the total number registered always stays
> bounded, but we're still talking about extreme abuse of the atexit
> API, and I think optimizing this case without an argument that it's
> worthwhile would probably be premature optimization.

It is probably not worth for optimization, I agree. It is more a
question of safety. Have code fail early that is trapped in an
infinite loop of adding atexit handlers during exit.

> > would be good if we could restrict the maximal number of successful
> > calls to atexit during exit to 32. A calloc-free strategy could be to
> > save head to a tmp a the beginning of processing and to provide a
> > `struct fl` table on the stack of __funcs_on_exit.
> 
> I'm not sure how this would be better. It would be more predictable,
> but could also probably break some excessive but "valid" use (like
> a huge chain of ctors getting called from an atexit handler and all
> registering dtors).

I think this is really excessive and probably very poor design. atexit
should not be abused to make an entry into the list on a per-object
base.

No application should expect to be able to submit more than 32
handlers. After that atexit is allowed to fail, so everything more
than that is not portable. I don't advocate to keep strictly on the 32
bound (as we do for at_quick_exit), but once the process has entered
the exit procedure, there should be pressure to get things terminated.

> > > A similar improvement could be done for at_quick_exit. There I think
> > > it makes not much sense accepting new handlers during processing. So
> > > the strategy could just be
> > > 
> > >   (i) Take the lock and save count in a tmp
> > >   (ii) set count to 32
> > >   (iii) unlock
> > >   (iv) do the processing
> > > 
> > > But looking at it, I think that one can even be more improved. We
> > > could just use count as atomic, and so we wouldn't need a lock.
> > > I can prepare a patch for that, if you like.
> > 
> > Since this is fairly simple, I just did it. Please find a new version
> > attached. (too much changes, a patch makes no sense.)  This compiles,
> > but I have no code to test this.
> 
> The current program in musl is not to treat replacements of locks by
> an atomics as an improvement, but rather the opposite.

Ok, I wasn't aware of it.

For the code in question, I am not sure that it is more complicated
than the LOCK/UNLOCK/re-LOCK scheme of the current implementation.

Generally, it is perhaps worth thinking to omit the LOCK/UNLOCK in
the __funcs_... loops altogether. This is a context that is guaranteed
to be single threaded, I think.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] fix atexit when it is called from an atexit handler

[Szabolcs Nagy]

* Jens Gustedt <jens.gustedt@inria.fr> [2015-07-23 23:51:53 +0200]:
> Am Donnerstag, den 23.07.2015, 15:58 -0400 schrieb Rich Felker:
> > On Thu, Jul 23, 2015 at 09:19:13AM +0200, Jens Gustedt wrote:
> > > would be good if we could restrict the maximal number of successful
> > > calls to atexit during exit to 32. A calloc-free strategy could be to
> > > save head to a tmp a the beginning of processing and to provide a
> > > `struct fl` table on the stack of __funcs_on_exit.
> > 
> > I'm not sure how this would be better. It would be more predictable,
> > but could also probably break some excessive but "valid" use (like
> > a huge chain of ctors getting called from an atexit handler and all
> > registering dtors).
> 
> I think this is really excessive and probably very poor design. atexit
> should not be abused to make an entry into the list on a per-object
> base.
> 
> No application should expect to be able to submit more than 32
> handlers. After that atexit is allowed to fail, so everything more
> than that is not portable. I don't advocate to keep strictly on the 32
> bound (as we do for at_quick_exit), but once the process has entered
> the exit procedure, there should be pressure to get things terminated.
> 

$ printf '
set breakpoint pending on
break __cxa_atexit
commands
frame 0
continue
end
run
' |gdb clang 2>/dev/null |grep '^Breakpoint' |wc -l
610

i.e. clang registers 610 atexit handlers.
(in case you wonder: gcc registers 2 including do_fini
of the musl runtime)

i don't find atexit after exit dangerous: it is
a programmer error if there are a lot of atexit
calls, not an input dependent dos attack surface.

Re: [PATCH] fix atexit when it is called from an atexit handler

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

Am Freitag, den 24.07.2015, 02:16 +0200 schrieb Szabolcs Nagy:
> * Jens Gustedt <jens.gustedt@inria.fr> [2015-07-23 23:51:53 +0200]:
> > Am Donnerstag, den 23.07.2015, 15:58 -0400 schrieb Rich Felker:
> > > On Thu, Jul 23, 2015 at 09:19:13AM +0200, Jens Gustedt wrote:
> > > > would be good if we could restrict the maximal number of successful
> > > > calls to atexit during exit to 32. A calloc-free strategy could be to
> > > > save head to a tmp a the beginning of processing and to provide a
> > > > `struct fl` table on the stack of __funcs_on_exit.
> > > 
> > > I'm not sure how this would be better. It would be more predictable,
> > > but could also probably break some excessive but "valid" use (like
> > > a huge chain of ctors getting called from an atexit handler and all
> > > registering dtors).
> > 
> > I think this is really excessive and probably very poor design. atexit
> > should not be abused to make an entry into the list on a per-object
> > base.
> > 
> > No application should expect to be able to submit more than 32
> > handlers. After that atexit is allowed to fail, so everything more
> > than that is not portable. I don't advocate to keep strictly on the 32
> > bound (as we do for at_quick_exit), but once the process has entered
> > the exit procedure, there should be pressure to get things terminated.
> > 
> 
> $ printf '
> set breakpoint pending on
> break __cxa_atexit
> commands
> frame 0
> continue
> end
> run
> ' |gdb clang 2>/dev/null |grep '^Breakpoint' |wc -l
> 610

interesting

> i.e. clang registers 610 atexit handlers.

hm, I don't think these are atexit handlers proper. I looked at the
addresses (sort -u | less), most of them are different, so they are probably
direct calls to __cxa_atexit, not to atexit.

> (in case you wonder: gcc registers 2 including do_fini
> of the musl runtime)
> 
> i don't find atexit after exit dangerous: it is
> a programmer error if there are a lot of atexit
> calls,

that's what I meant

> not an input dependent dos attack surface.

I am not an expert in these, but AFAIR all function pointers that are
stored in predictable places are attack surfaces.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] fix atexit when it is called from an atexit handler

[Szabolcs Nagy]

* Jens Gustedt <jens.gustedt@inria.fr> [2015-07-24 08:52:23 +0200]:
> Am Freitag, den 24.07.2015, 02:16 +0200 schrieb Szabolcs Nagy:
> > 
> > $ printf '
> > set breakpoint pending on
> > break __cxa_atexit
> > commands
> > frame 0
> > continue
> > end
> > run
> > ' |gdb clang 2>/dev/null |grep '^Breakpoint' |wc -l
> > 610
> 
> interesting
> 
> > i.e. clang registers 610 atexit handlers.
> 
> hm, I don't think these are atexit handlers proper. I looked at the
> addresses (sort -u | less), most of them are different, so they are probably
> direct calls to __cxa_atexit, not to atexit.

they are destructors for static objects, but those are
atexit handlers too: they get into the same list.

(the c++ standard does not guarantee limits for static
objects with destructors even though it specifies the
limit for atexit handlers and requires interleaved
execution of atexit handlers and destructors, so the
only reasonable implementation is to use the same
mechanism for them.

in theory we could have a fixed array of 32 list items
for c atexit handlers and always malloc items for c++,
but that seems extra work just to provide the minimal
possible guarantees of the standards.)

> > i don't find atexit after exit dangerous: it is
> > a programmer error if there are a lot of atexit
> > calls,
> 
> that's what I meant
> 
> > not an input dependent dos attack surface.
> 
> I am not an expert in these, but AFAIR all function pointers that are
> stored in predictable places are attack surfaces.

if there is memory corruption and an attacker controls
the pointers then it is an attack surface, but without
memory corruption the number of atexit handlers
registered rarely depends on input at runtime, it's a
design decision by the programmer.

Re: [PATCH] fix atexit when it is called from an atexit handler

[Rich Felker]

On Fri, Jul 24, 2015 at 08:52:23AM +0200, Jens Gustedt wrote:
> > not an input dependent dos attack surface.
> 
> I am not an expert in these, but AFAIR all function pointers that are
> stored in predictable places are attack surfaces.

This is a valid position, but I would distinguish interface-level
attack surfaces, in the form of interfaces that are processing
attacker-controlled data, from secondary attack surfaces that are only
accessible once you have already exploited a bug that yields undefined
behavior. These function pointers are the latter.

Rich

Re: [PATCH] fix atexit when it is called from an atexit handler

[Rich Felker]

On Thu, Jul 23, 2015 at 05:07:21AM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2015-07-22 21:18:16 -0400]:
> > On Thu, Jul 23, 2015 at 01:44:06AM +0200, Szabolcs Nagy wrote:
> > >  void __funcs_on_exit()
> > >  {
> > >  	int i;
> > >  	void (*func)(void *), *arg;
> > >  	LOCK(lock);
> > > -	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
> > > -		if (!head->f[i]) continue;
> > > +	for (;;) {
> > > +		i = next();
> > > +		if (i<0) break;
> > >  		func = head->f[i];
> > >  		arg = head->a[i];
> > >  		head->f[i] = 0;
> > 
> > I agree that this change should be made, but I don't like the
> > particulars of the patch. It seems to increase exit handler runtime to
> > O(n²) even in the case where no atexit is happening during exit, and
> > it's not very elegant. I think a simpler solution would be to reset i
> > to COUNT after calling f[i] if either (1) head has changed, or (2)
> > i<COUNT-1 and f[i+1] is nonzero. Does this sound correct/better?
> 
> (2) should be 'f[i] is nonzero'.
> 
> i didnt think about detecting the change of head,
> but it is simpler and better for the common case.

> >From 63c2b2d58b2ec45df67927b8959ea8af6dd578ed Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@port70.net>
> Date: Wed, 22 Jul 2015 23:05:57 +0000
> Subject: [PATCH] fix atexit when it is called from an atexit handler
> 
> The old code accepted atexit handlers after exit, but did not run
> them.  C11 seems to explicitly allow atexit to fail in this case,
> but this situation can easily come up in C++ if a destructor has
> local static object with a destructor so it should be handled.
> 
> Restart iterating the list of atexit handlers if new handlers are
> added during a call.
> 
> Note that the memory usage can grow linearly with the overall
> number of registered atexit handlers instead of with the worst
> case list length. (This only matters if atexit handlers keep
> registering atexit handlers which should not happen in practice).
> ---
>  src/exit/atexit.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/exit/atexit.c b/src/exit/atexit.c
> index be82718..75f7511 100644
> --- a/src/exit/atexit.c
> +++ b/src/exit/atexit.c
> @@ -17,6 +17,7 @@ static volatile int lock[2];
>  void __funcs_on_exit()
>  {
>  	int i;
> +	struct fl *old;
>  	void (*func)(void *), *arg;
>  	LOCK(lock);
>  	for (; head; head=head->next) for (i=COUNT-1; i>=0; i--) {
> @@ -24,9 +25,12 @@ void __funcs_on_exit()
>  		func = head->f[i];
>  		arg = head->a[i];
>  		head->f[i] = 0;
> +		old = head;
>  		UNLOCK(lock);
>  		func(arg);
>  		LOCK(lock);
> +		if (head != old || head->f[i])
> +			i = COUNT;
>  	}
>  }

I've committed the alternate version we discussed on IRC that avoids
scanning over the array to find the current position by using an extra
int of global state, the 'next slot' in head, which is protected by
the existing lock. This code is not heavily tested, so testing would
be welcome.

Rich