From patchwork Mon Jul 1 20:59:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 168221 Delivered-To: patch@linaro.org Received: by 2002:a92:4782:0:0:0:0:0 with SMTP id e2csp3376263ilk; Mon, 1 Jul 2019 13:59:23 -0700 (PDT) X-Google-Smtp-Source: APXvYqw3oFHM5u6bBfhAoxqRNKxbRU23VH7hFVuffbqwT9N+yWcoYLYEdetT3ESLibacFzVvB059 X-Received: by 2002:a65:4186:: with SMTP id a6mr26756218pgq.164.1562014763146; Mon, 01 Jul 2019 13:59:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1562014763; cv=none; d=google.com; s=arc-20160816; b=QYn1oEZy6G1plctoU7UquNmuQE3lEq1vwICRba+/i97bLPfH+yyMvZlSyKH3WNGotH NrVnNyeqaK2f+c/C+TldpeYv8OjLfTTAuzyLC46JvFcF6tD5q4ARVg1snZB3eSour9VT AEr900S3SmIRdAqfq/BKnx5BxYplO8QLpc0vHRIf+QfCyKQ9y5w39dA1EFjFkq1Iso/0 hBuQGq0UJ0l3aMkDqHJHFM/7zMZKlbLRyl5YTUFrLtZFlYCEtmyic1Bg5tjG89jLrmIm Dsi1O/W8JBVdYpnA53cZsaK2TD951bRRg8G4ipaLY60r9JH1AupIbhWGHZSAdfKBhgEg fG7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:subject:to:from:dkim-signature:delivered-to:sender :list-help:list-post:list-archive:list-subscribe:list-unsubscribe :list-id:precedence:mailing-list:dkim-signature:domainkey-signature; bh=CFB7jo82KR2hOWS6S6RZSEU3N7VPwKWmixeoYFYYdAs=; b=TbN+N6NwcYAWmrlKz+lAqU8iTAi6XJj0lhUQmJ6aeRhvpwg2vLVNHlNGeHni126WDz n9Zh2s3mGqDtAmb5BwU+Ktg0LIH4MX9IuhMVOOEm7/XVKsx350EQHWpq6DeYxy++A8Jv KbqooEeSkbkWtrpUvywriANyErOGCAoP4MawUOTlDVZvlY4YZ4iL3kWRHizkuV9gLsXo y7VoTC0YU0CFv3Dk2m66fHx5RvIy6RjcsiKSeIRNyoLetVidtQnFhf8qSjAcqj8fFSNe 0gGHhrU/sPH0OG6rDbVAoGkZ+oAB+C9z43CpibFBX2TtXt+MYG23NLeNKUOcwwhxomZu g4tw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=TC9s6jwq; dkim=pass header.i=@linaro.org header.s=google header.b=hIab2bf0; spf=pass (google.com: domain of libc-alpha-return-103392-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-103392-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id s14si10755376pgs.254.2019.07.01.13.59.22 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Jul 2019 13:59:23 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-103392-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=TC9s6jwq; dkim=pass header.i=@linaro.org header.s=google header.b=hIab2bf0; spf=pass (google.com: domain of libc-alpha-return-103392-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-103392-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; q=dns; s= default; b=IeaYkFm92Ul5nUZEiFRyJYTR8mAomDwAyPWzGjI8/ELTrmHA3b3cA GW3H1/XQBU3QvNXCO0YYlZCaUcfjVm88e02DYASvYXAunffmnieqxpFCIz/OEHeI KOKgp4pMbBuq8ZSnhkLs0SiM9Ems8l7JlgaKtGgcRUC2sZk7L5yTvg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; s=default; bh=D8Z0cPYvG9+zpN5jWIYjv5ftc9g=; b=TC9s6jwqDS4e+4SomxWStn1V1/sU 47IFQ+Z05rH0s8PgjAk7ayX+9622YJrkGJOAgQjJ1qrH21uwy4RnaYLqGicfkBr2 d0w2VkT1UG+sPdZoz5of/YGArBeiAfqD9IryRaQyp7LGhDorpvBla6MOuWSEcfeR PSi9XioKVgOQXbY= Received: (qmail 38378 invoked by alias); 1 Jul 2019 20:59:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 38226 invoked by uid 89); 1 Jul 2019 20:59:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Spam-Relays-External:209.85.217.65, H*RU:209.85.217.65 X-HELO: mail-vs1-f65.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id; bh=CFB7jo82KR2hOWS6S6RZSEU3N7VPwKWmixeoYFYYdAs=; b=hIab2bf0qS51XzXNMmVZQs8kKrZln1hs/hfaUUuF8ARtDFUSq4YUhtZ/yjCwdPhBz3 Afpm/yOQawZBvDou8gX+zvDczkTWkGH+d2jku0eghzke1W5+uJd+d1AzR9kLxLuT7q/w wGWfyFy5SsAqV5bs5Fe6RL/xlW/Qdatlg7kbYuRIfPvavPQCXFH1Yi+X882eBUfgzeYv o5qLLO/4+30ZL7ki+a2gytUwTsUKmpKPs4zHOoAA2hDjLGcNslksRw//QaIW4kMXadex K5Xd4YQ2seHVw6+eL4XgRHCrypHYJ3tB15K0wuUydEv6PCVNtCivzFNhSETdrruNn/qb Ng8g== Return-Path: From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [RFC] stdlib: Make atexit to not act as __cxa_atexit Date: Mon, 1 Jul 2019 17:59:04 -0300 Message-Id: <20190701205904.22287-1-adhemerval.zanella@linaro.org> This is RFC patch to address the issue brought in recent discussion regarding atexit and shared libraries [1] [2]. As indicated in the libc-alpha discussion, the issue is since atexit uses __cxa_atexit its interaction __cxa_finalize could lead to atexit handlers being executed in a different order than the expected one. The github project gives a small example that triggers it [3]. The changes I could come with changes slight the atexit semantic as described in my last email [4]. So basically the changes are: 1. Add the __atexit symbol which is linked as __cxa_finalize in static mode (so __dso_handle is correctly set). The __atexit symbol adds an ef_at exit_function entry on __exit_funcs, different than an ef_cxa one from __cxa_atexit. Old binaries would still call __cxa_atexit, so we do not actually need to add a compat symbol. 2. Make __cxa_finalize set ef_at handlers to ef_at_finalize. This is to for dlclose to mark the atexit handlers as free, so a subsequent __cxa_finalize won't run potentially unmmap code. 3. exit is the only way where atexit handlers are actually called. Unloading a library through dlclose does not issue the atexit anymore (this is the main semantic change). I have not opened a bug report about it because I am not sure from the previous discussion this is indeed a bug or an expected side-effect implementing atexit based on __cxa_atexit. This is also only happens when atexit is created from a shared library (which has its own __dso_handle value) on some specific scenarios where the unloading order is changed. The RFC also has a *lot* of missing parts (tests, abifiles, comments, CL). [1] https://sourceware.org/ml/libc-alpha/2019-06/msg00229.html [2] https://sourceware.org/ml/libc-help/2019-06/msg00025.html [3] https://github.com/mulle-nat/ld-so-breakage [4] https://sourceware.org/ml/libc-alpha/2019-06/msg00231.html --- dlfcn/dlclose.c | 4 +- dlfcn/modatexit.c | 11 +- dlfcn/tstatexit.c | 30 +--- include/stdlib.h | 3 + stdlib/Versions | 4 + stdlib/atexit.c | 2 +- stdlib/cxa_atexit.c | 28 +++- stdlib/cxa_finalize.c | 133 +++++++++++------- stdlib/exit.c | 3 +- stdlib/exit.h | 7 +- .../unix/sysv/linux/x86_64/64/libc.abilist | 1 + 11 files changed, 141 insertions(+), 85 deletions(-) -- 2.17.1 diff --git a/dlfcn/dlclose.c b/dlfcn/dlclose.c index 98e1375c43..c9a1e643f3 100644 --- a/dlfcn/dlclose.c +++ b/dlfcn/dlclose.c @@ -43,7 +43,9 @@ __dlclose (void *handle) return _dlfcn_hook->dlclose (handle); # endif - return _dlerror_run (dlclose_doit, handle) ? -1 : 0; + int r = _dlerror_run (dlclose_doit, handle) ? -1 : 0; + __atexit_cleanup (); + return r; } # ifdef SHARED strong_alias (__dlclose, dlclose) diff --git a/dlfcn/modatexit.c b/dlfcn/modatexit.c index 118ccf5459..8b16e0ba8f 100644 --- a/dlfcn/modatexit.c +++ b/dlfcn/modatexit.c @@ -21,11 +21,14 @@ int global; int *ip; -extern void dummy (void); -extern void foo (void *p); +/* glibc function for registering DSO-specific unload functions. */ +extern int __cxa_atexit (void (*func) (void *), void *arg, void *d); + +/* Hidden compiler handle to this shared object. */ +extern void *__dso_handle __attribute__ ((__weak__)); void -dummy (void) +dummy (void *arg) { printf ("This is %s\n", __FUNCTION__); *ip = global = 1; @@ -36,6 +39,6 @@ void foo (void *p) { printf ("This is %s\n", __FUNCTION__); - atexit (dummy); + __cxa_atexit (dummy, NULL, __dso_handle); ip = p; } diff --git a/dlfcn/tstatexit.c b/dlfcn/tstatexit.c index c8cb272ce0..1f6037cb47 100644 --- a/dlfcn/tstatexit.c +++ b/dlfcn/tstatexit.c @@ -19,6 +19,8 @@ #include #include +#include +#include int main (void) @@ -28,35 +30,15 @@ main (void) void (*fp) (void *); int v = 0; - h = dlopen (fname, RTLD_NOW); - if (h == NULL) - { - printf ("cannot open \"%s\": %s\n", fname, dlerror ()); - exit (1); - } + h = xdlopen (fname, RTLD_NOW); - fp = dlsym (h, "foo"); - if (fp == NULL) - { - printf ("cannot find \"foo\": %s\n", dlerror ()); - exit (1); - } + fp = xdlsym (h, "foo"); fp (&v); - if (dlclose (h) != 0) - { - printf ("cannot close \"%s\": %s\n", fname, dlerror ()); - exit (1); - } + xdlclose (h); - if (v != 1) - { - puts ("module unload didn't change `v'"); - exit (1); - } - - puts ("finishing now"); + TEST_COMPARE (v, 1); return 0; } diff --git a/include/stdlib.h b/include/stdlib.h index 114e12d255..3bf47ea49d 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -103,6 +103,9 @@ extern int __on_exit (void (*__func) (int __status, void *__arg), void *__arg); extern int __cxa_atexit (void (*func) (void *), void *arg, void *d); libc_hidden_proto (__cxa_atexit); +extern int __atexit (void (*func) (void), void *); +extern void __atexit_cleanup (void); + extern int __cxa_thread_atexit_impl (void (*func) (void *), void *arg, void *d); extern void __call_tls_dtors (void) diff --git a/stdlib/Versions b/stdlib/Versions index 9e665d4c26..9a9d911621 100644 --- a/stdlib/Versions +++ b/stdlib/Versions @@ -136,6 +136,9 @@ libc { strtof32; strtof64; strtof32x; strtof32_l; strtof64_l; strtof32x_l; } + GLIBC_2.29 { + __atexit; + } GLIBC_PRIVATE { # functions which have an additional interface since they are # are cancelable. @@ -146,5 +149,6 @@ libc { __libc_secure_getenv; __call_tls_dtors; __strtof_nan; __strtod_nan; __strtold_nan; + __atexit_cleanup; } } diff --git a/stdlib/atexit.c b/stdlib/atexit.c index 5671a43926..de8570e3bf 100644 --- a/stdlib/atexit.c +++ b/stdlib/atexit.c @@ -43,5 +43,5 @@ attribute_hidden #endif atexit (void (*func) (void)) { - return __cxa_atexit ((void (*) (void *)) func, NULL, __dso_handle); + return __atexit (func, __dso_handle); } diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c index 5a39778959..88ee811ac3 100644 --- a/stdlib/cxa_atexit.c +++ b/stdlib/cxa_atexit.c @@ -23,7 +23,6 @@ #include "exit.h" #include -#undef __cxa_atexit /* See concurrency notes in stdlib/exit.h where this lock is declared. */ __libc_lock_define_initialized (, __exit_funcs_lock) @@ -71,6 +70,33 @@ __cxa_atexit (void (*func) (void *), void *arg, void *d) } libc_hidden_def (__cxa_atexit) +int +__atexit (void (*func) (void), void *dso_handle) +{ + struct exit_function *new; + + /* As a QoI issue we detect NULL early with an assertion instead + of a SIGSEGV at program exit when the handler is run (bug 20544). */ + assert (func != NULL); + + __libc_lock_lock (__exit_funcs_lock); + new = __new_exitfn (&__exit_funcs); + + if (new == NULL) + { + __libc_lock_unlock (__exit_funcs_lock); + return -1; + } + +#ifdef PTR_MANGLE + PTR_MANGLE (func); +#endif + new->func.at.fn = func; + new->func.at.dso_handle = dso_handle; + new->flavor = ef_at; + __libc_lock_unlock (__exit_funcs_lock); + return 0; +} static struct exit_function_list initial; struct exit_function_list *__exit_funcs = &initial; diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c index e31b23467c..aeb364eb9b 100644 --- a/stdlib/cxa_finalize.c +++ b/stdlib/cxa_finalize.c @@ -22,6 +22,60 @@ #include #include +static bool +handle_cxa (struct exit_function *f) +{ + const uint64_t check = __new_exitfn_called; + + void (*cxafn) (void *arg, int status) = f->func.cxa.fn; + void *cxaarg = f->func.cxa.arg; + + /* We don't want to run this cleanup more than once. The Itanium + C++ ABI requires that multiple calls to __cxa_finalize not + result in calling termination functions more than once. One + potential scenario where that could happen is with a concurrent + dlclose and exit, where the running dlclose must at some point + release the list lock, an exiting thread may acquire it, and + without setting flavor to ef_free, might re-run this destructor + which could result in undefined behaviour. Therefore we must + set flavor to ef_free to avoid calling this destructor again. + Note that the concurrent exit must also take the dynamic loader + lock (for library finalizer processing) and therefore will + block while dlclose completes the processing of any in-progress + exit functions. Lastly, once we release the list lock for the + entry marked ef_free, we must not read from that entry again + since it may have been reused by the time we take the list lock + again. Lastly the detection of new registered exit functions is + based on a monotonically incrementing counter, and there is an + ABA if between the unlock to run the exit function and the + re-lock after completion the user registers 2^64 exit functions, + the implementation will not detect this and continue without + executing any more functions. + + One minor issue remains: A registered exit function that is in + progress by a call to dlclose() may not completely finish before + the next registered exit function is run. This may, according to + some readings of POSIX violate the requirement that functions + run in effective LIFO order. This should probably be fixed in a + future implementation to ensure the functions do not run in + parallel. */ + f->flavor = ef_free; + +#ifdef PTR_DEMANGLE + PTR_DEMANGLE (cxafn); +#endif + /* Unlock the list while we call a foreign function. */ + __libc_lock_unlock (__exit_funcs_lock); + cxafn (cxaarg, 0); + __libc_lock_lock (__exit_funcs_lock); + + /* It is possible that that last exit function registered + more exit functions. Start the loop over. */ + if (__glibc_unlikely (check != __new_exitfn_called)) + return true; + return false; +} + /* If D is non-NULL, call all functions registered with `__cxa_atexit' with the same dso handle. Otherwise, if D is NULL, call all of the registered handlers. */ @@ -35,59 +89,19 @@ __cxa_finalize (void *d) restart: for (funcs = __exit_funcs; funcs; funcs = funcs->next) { - struct exit_function *f; - - for (f = &funcs->fns[funcs->idx - 1]; f >= &funcs->fns[0]; --f) - if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa) - { - const uint64_t check = __new_exitfn_called; - void (*cxafn) (void *arg, int status) = f->func.cxa.fn; - void *cxaarg = f->func.cxa.arg; - - /* We don't want to run this cleanup more than once. The Itanium - C++ ABI requires that multiple calls to __cxa_finalize not - result in calling termination functions more than once. One - potential scenario where that could happen is with a concurrent - dlclose and exit, where the running dlclose must at some point - release the list lock, an exiting thread may acquire it, and - without setting flavor to ef_free, might re-run this destructor - which could result in undefined behaviour. Therefore we must - set flavor to ef_free to avoid calling this destructor again. - Note that the concurrent exit must also take the dynamic loader - lock (for library finalizer processing) and therefore will - block while dlclose completes the processing of any in-progress - exit functions. Lastly, once we release the list lock for the - entry marked ef_free, we must not read from that entry again - since it may have been reused by the time we take the list lock - again. Lastly the detection of new registered exit functions is - based on a monotonically incrementing counter, and there is an - ABA if between the unlock to run the exit function and the - re-lock after completion the user registers 2^64 exit functions, - the implementation will not detect this and continue without - executing any more functions. - - One minor issue remains: A registered exit function that is in - progress by a call to dlclose() may not completely finish before - the next registered exit function is run. This may, according to - some readings of POSIX violate the requirement that functions - run in effective LIFO order. This should probably be fixed in a - future implementation to ensure the functions do not run in - parallel. */ - f->flavor = ef_free; - -#ifdef PTR_DEMANGLE - PTR_DEMANGLE (cxafn); -#endif - /* Unlock the list while we call a foreign function. */ - __libc_lock_unlock (__exit_funcs_lock); - cxafn (cxaarg, 0); - __libc_lock_lock (__exit_funcs_lock); - - /* It is possible that that last exit function registered - more exit functions. Start the loop over. */ - if (__glibc_unlikely (check != __new_exitfn_called)) + for (struct exit_function *f = &funcs->fns[funcs->idx - 1]; + f >= &funcs->fns[0]; --f) + { + if ((d == NULL || d == f->func.cxa.dso_handle) + && f->flavor == ef_cxa) + if (handle_cxa (f)) goto restart; - } + + /* Mark ef_at_exit handlers are handled by __cxa_finalize. */ + if ((d == NULL || d == f->func.at.dso_handle) + && f->flavor == ef_at) + f->flavor = ef_at_finalize; + } } /* Also remove the quick_exit handlers, but do not call them. */ @@ -108,3 +122,18 @@ __cxa_finalize (void *d) #endif __libc_lock_unlock (__exit_funcs_lock); } + +void +__atexit_cleanup (void) +{ + __libc_lock_lock (__exit_funcs_lock); + for (struct exit_function_list * funcs = __exit_funcs; funcs != NULL; + funcs = funcs->next) + { + for (struct exit_function *f = &funcs->fns[funcs->idx - 1]; + f >= &funcs->fns[0]; --f) + if (f->flavor == ef_at_finalize) + f->flavor = ef_free; + } + __libc_lock_unlock (__exit_funcs_lock); +} diff --git a/stdlib/exit.c b/stdlib/exit.c index 49d12d9952..8352760fae 100644 --- a/stdlib/exit.c +++ b/stdlib/exit.c @@ -91,7 +91,8 @@ __run_exit_handlers (int status, struct exit_function_list **listp, onfct (status, f->func.on.arg); break; case ef_at: - atfct = f->func.at; + case ef_at_finalize: + atfct = f->func.at.fn; #ifdef PTR_DEMANGLE PTR_DEMANGLE (atfct); #endif diff --git a/stdlib/exit.h b/stdlib/exit.h index 6fab49c94f..a1d4860206 100644 --- a/stdlib/exit.h +++ b/stdlib/exit.h @@ -28,6 +28,7 @@ enum ef_us, ef_on, ef_at, + ef_at_finalize, ef_cxa }; @@ -38,7 +39,11 @@ struct exit_function long int flavor; union { - void (*at) (void); + struct + { + void (*fn) (void); + void *dso_handle; + } at; struct { void (*fn) (int status, void *arg); diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist index 59f85d9373..699fd105de 100644 --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist @@ -1895,6 +1895,7 @@ GLIBC_2.28 thrd_current F GLIBC_2.28 thrd_equal F GLIBC_2.28 thrd_sleep F GLIBC_2.28 thrd_yield F +GLIBC_2.29 __atexit F GLIBC_2.29 getcpu F GLIBC_2.29 posix_spawn_file_actions_addchdir_np F GLIBC_2.29 posix_spawn_file_actions_addfchdir_np F