* [PATCH] fix atexit when it is called from an atexit handler @ 2015-07-22 23:44 Szabolcs Nagy 2015-07-23 1:18 ` Rich Felker 0 siblings, 1 reply; 12+ messages in thread From: Szabolcs Nagy @ 2015-07-22 23:44 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-22 23:44 [PATCH] fix atexit when it is called from an atexit handler Szabolcs Nagy @ 2015-07-23 1:18 ` Rich Felker 2015-07-23 3:07 ` Szabolcs Nagy 0 siblings, 1 reply; 12+ messages in thread From: Rich Felker @ 2015-07-23 1:18 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 1:18 ` Rich Felker @ 2015-07-23 3:07 ` Szabolcs Nagy 2015-07-23 5:54 ` Jens Gustedt 2015-07-24 21:25 ` Rich Felker 0 siblings, 2 replies; 12+ messages in thread From: Szabolcs Nagy @ 2015-07-23 3:07 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 3:07 ` Szabolcs Nagy @ 2015-07-23 5:54 ` Jens Gustedt 2015-07-23 7:19 ` Jens Gustedt 2015-07-24 21:25 ` Rich Felker 1 sibling, 1 reply; 12+ messages in thread From: Jens Gustedt @ 2015-07-23 5:54 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 5:54 ` Jens Gustedt @ 2015-07-23 7:19 ` Jens Gustedt 2015-07-23 19:58 ` Rich Felker 0 siblings, 1 reply; 12+ messages in thread From: Jens Gustedt @ 2015-07-23 7:19 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 7:19 ` Jens Gustedt @ 2015-07-23 19:58 ` Rich Felker 2015-07-23 21:51 ` Jens Gustedt 0 siblings, 1 reply; 12+ messages in thread From: Rich Felker @ 2015-07-23 19:58 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 19:58 ` Rich Felker @ 2015-07-23 21:51 ` Jens Gustedt 2015-07-24 0:16 ` Szabolcs Nagy 0 siblings, 1 reply; 12+ messages in thread From: Jens Gustedt @ 2015-07-23 21:51 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 21:51 ` Jens Gustedt @ 2015-07-24 0:16 ` Szabolcs Nagy 2015-07-24 6:52 ` Jens Gustedt 0 siblings, 1 reply; 12+ messages in thread From: Szabolcs Nagy @ 2015-07-24 0:16 UTC (permalink / raw) To: musl * 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-24 0:16 ` Szabolcs Nagy @ 2015-07-24 6:52 ` Jens Gustedt 2015-07-24 10:53 ` Szabolcs Nagy 2015-07-24 16:01 ` Rich Felker 0 siblings, 2 replies; 12+ messages in thread From: Jens Gustedt @ 2015-07-24 6:52 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-24 6:52 ` Jens Gustedt @ 2015-07-24 10:53 ` Szabolcs Nagy 2015-07-24 16:01 ` Rich Felker 1 sibling, 0 replies; 12+ messages in thread From: Szabolcs Nagy @ 2015-07-24 10:53 UTC (permalink / raw) To: musl * 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-24 6:52 ` Jens Gustedt 2015-07-24 10:53 ` Szabolcs Nagy @ 2015-07-24 16:01 ` Rich Felker 1 sibling, 0 replies; 12+ messages in thread From: Rich Felker @ 2015-07-24 16:01 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fix atexit when it is called from an atexit handler 2015-07-23 3:07 ` Szabolcs Nagy 2015-07-23 5:54 ` Jens Gustedt @ 2015-07-24 21:25 ` Rich Felker 1 sibling, 0 replies; 12+ messages in thread From: Rich Felker @ 2015-07-24 21:25 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-07-24 21:25 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-22 23:44 [PATCH] fix atexit when it is called from an atexit handler Szabolcs Nagy 2015-07-23 1:18 ` Rich Felker 2015-07-23 3:07 ` Szabolcs Nagy 2015-07-23 5:54 ` Jens Gustedt 2015-07-23 7:19 ` Jens Gustedt 2015-07-23 19:58 ` Rich Felker 2015-07-23 21:51 ` Jens Gustedt 2015-07-24 0:16 ` Szabolcs Nagy 2015-07-24 6:52 ` Jens Gustedt 2015-07-24 10:53 ` Szabolcs Nagy 2015-07-24 16:01 ` Rich Felker 2015-07-24 21:25 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).