From 96a6162e1925d4bd1dfc6f4167d75f396c0dbe4c Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 4 Oct 2019 15:04:13 -0400 Subject: [PATCH] Fix: fd tracker: provide async-signal-safety for close wrapper close(3) is part of the async-signal-safe functions. Therefore, it is expected that the close wrapper provided by liblttng-ust-fd-tracker behaves in a async-signal-safe way. Use a similar strategy as ust_lock() does: disable signals when taking and releasing the lock, and keep track of nesting with a TLS variable. This ensures signals are restored to their original state when close(3) ends up being invoked. Fixes: #1199 Signed-off-by: Mathieu Desnoyers --- liblttng-ust-comm/lttng-ust-fd-tracker.c | 72 +++++++++++++++++------- 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/liblttng-ust-comm/lttng-ust-fd-tracker.c b/liblttng-ust-comm/lttng-ust-fd-tracker.c index 1118163a..9659f349 100644 --- a/liblttng-ust-comm/lttng-ust-fd-tracker.c +++ b/liblttng-ust-comm/lttng-ust-fd-tracker.c @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include #include #include @@ -75,9 +77,11 @@ static int ust_safe_guard_saved_cancelstate; /* * Track whether we are within lttng-ust or application, for close - * system call override by LD_PRELOAD library. + * system call override by LD_PRELOAD library. This also tracks whether + * we are invoking close() from a signal handler nested on an + * application thread. */ -static DEFINE_URCU_TLS(int, thread_fd_tracking); +static DEFINE_URCU_TLS(int, ust_fd_mutex_nest); /* fd_set used to book keep fd being used by lttng-ust. */ static fd_set *lttng_fd_set; @@ -90,7 +94,7 @@ static int init_done; */ void lttng_ust_fixup_fd_tracker_tls(void) { - asm volatile ("" : : "m" (URCU_TLS(thread_fd_tracking))); + asm volatile ("" : : "m" (URCU_TLS(ust_fd_mutex_nest))); } /* @@ -134,37 +138,63 @@ void lttng_ust_init_fd_tracker(void) void lttng_ust_lock_fd_tracker(void) { + sigset_t sig_all_blocked, orig_mask; int ret, oldstate; ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); if (ret) { ERR("pthread_setcancelstate: %s", strerror(ret)); } - URCU_TLS(thread_fd_tracking) = 1; - /* - * Ensure the compiler don't move the store after the close() - * call in case close() would be marked as leaf. - */ - cmm_barrier(); - pthread_mutex_lock(&ust_safe_guard_fd_mutex); - ust_safe_guard_saved_cancelstate = oldstate; + sigfillset(&sig_all_blocked); + ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); + if (ret) { + ERR("pthread_sigmask: %s", strerror(ret)); + } + if (!URCU_TLS(ust_fd_mutex_nest)++) { + /* + * Ensure the compiler don't move the store after the close() + * call in case close() would be marked as leaf. + */ + cmm_barrier(); + pthread_mutex_lock(&ust_safe_guard_fd_mutex); + ust_safe_guard_saved_cancelstate = oldstate; + } + ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); + if (ret) { + ERR("pthread_sigmask: %s", strerror(ret)); + } } void lttng_ust_unlock_fd_tracker(void) { + sigset_t sig_all_blocked, orig_mask; int ret, newstate, oldstate; + bool restore_cancel = false; - newstate = ust_safe_guard_saved_cancelstate; - pthread_mutex_unlock(&ust_safe_guard_fd_mutex); + sigfillset(&sig_all_blocked); + ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); + if (ret) { + ERR("pthread_sigmask: %s", strerror(ret)); + } /* * Ensure the compiler don't move the store before the close() * call, in case close() would be marked as leaf. */ cmm_barrier(); - URCU_TLS(thread_fd_tracking) = 0; - ret = pthread_setcancelstate(newstate, &oldstate); + if (!--URCU_TLS(ust_fd_mutex_nest)) { + newstate = ust_safe_guard_saved_cancelstate; + restore_cancel = true; + pthread_mutex_unlock(&ust_safe_guard_fd_mutex); + } + ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); + ERR("pthread_sigmask: %s", strerror(ret)); + } + if (restore_cancel) { + ret = pthread_setcancelstate(newstate, &oldstate); + if (ret) { + ERR("pthread_setcancelstate: %s", strerror(ret)); + } } } @@ -255,7 +285,7 @@ int lttng_ust_add_fd_to_tracker(int fd) * constructors. */ lttng_ust_init_fd_tracker(); - assert(URCU_TLS(thread_fd_tracking)); + assert(URCU_TLS(ust_fd_mutex_nest)); if (IS_FD_STD(fd)) { ret = dup_std_fd(fd); @@ -288,7 +318,7 @@ void lttng_ust_delete_fd_from_tracker(int fd) */ lttng_ust_init_fd_tracker(); - assert(URCU_TLS(thread_fd_tracking)); + assert(URCU_TLS(ust_fd_mutex_nest)); /* Not a valid fd. */ assert(IS_FD_VALID(fd)); /* Deleting an fd which was not set. */ @@ -318,7 +348,7 @@ int lttng_ust_safe_close_fd(int fd, int (*close_cb)(int fd)) * If called from lttng-ust, we directly call close without * validating whether the FD is part of the tracked set. */ - if (URCU_TLS(thread_fd_tracking)) + if (URCU_TLS(ust_fd_mutex_nest)) return close_cb(fd); lttng_ust_lock_fd_tracker(); @@ -354,7 +384,7 @@ int lttng_ust_safe_fclose_stream(FILE *stream, int (*fclose_cb)(FILE *stream)) * If called from lttng-ust, we directly call fclose without * validating whether the FD is part of the tracked set. */ - if (URCU_TLS(thread_fd_tracking)) + if (URCU_TLS(ust_fd_mutex_nest)) return fclose_cb(stream); fd = fileno(stream); @@ -417,7 +447,7 @@ int lttng_ust_safe_closefrom_fd(int lowfd, int (*close_cb)(int fd)) * If called from lttng-ust, we directly call close without * validating whether the FD is part of the tracked set. */ - if (URCU_TLS(thread_fd_tracking)) { + if (URCU_TLS(ust_fd_mutex_nest)) { for (i = lowfd; i < lttng_ust_max_fd; i++) { if (close_cb(i) < 0) { switch (errno) { -- 2.34.1