[lttng-dev] [PATCH lttng-ust v2 1/2] Force tracked fd to be bigger than STDERR_FILENO
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Tue Feb 6 13:27:28 UTC 2018
Both patches merged into master, 2.10, 2.9, thanks!
Mathieu
----- On Feb 5, 2018, at 5:58 PM, Jonathan Rajotte jonathan.rajotte-julien at efficios.com wrote:
> This allow ust to be proactive regarding std* fd manipulation done by
> external source.
>
> A good example of this is the "daemon" function that can dup2 statically
> the std* fd and close them silently if the were already used.
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> ---
> v2: No change.
> ---
> include/ust-fd.h | 2 +-
> liblttng-ust-comm/lttng-ust-comm.c | 51 +++++++++++++++---
> liblttng-ust-comm/lttng-ust-fd-tracker.c | 90 +++++++++++++++++++++++++++++++-
> liblttng-ust/lttng-ust-comm.c | 48 ++++++++++++++---
> liblttng-ust/lttng-ust-elf.c | 18 +++++--
> 5 files changed, 190 insertions(+), 19 deletions(-)
>
> diff --git a/include/ust-fd.h b/include/ust-fd.h
> index 032cac5..a015525 100644
> --- a/include/ust-fd.h
> +++ b/include/ust-fd.h
> @@ -27,7 +27,7 @@
> #include <stdio.h>
>
> void lttng_ust_init_fd_tracker(void);
> -void lttng_ust_add_fd_to_tracker(int fd);
> +int lttng_ust_add_fd_to_tracker(int fd);
> void lttng_ust_delete_fd_from_tracker(int fd);
> void lttng_ust_lock_fd_tracker(void);
> void lttng_ust_unlock_fd_tracker(void);
> diff --git a/liblttng-ust-comm/lttng-ust-comm.c
> b/liblttng-ust-comm/lttng-ust-comm.c
> index 9fe6d28..3d1c650 100644
> --- a/liblttng-ust-comm/lttng-ust-comm.c
> +++ b/liblttng-ust-comm/lttng-ust-comm.c
> @@ -598,7 +598,7 @@ ssize_t ustcomm_recv_channel_from_sessiond(int sock,
> {
> void *chan_data;
> ssize_t len, nr_fd;
> - int wakeup_fd;
> + int wakeup_fd, ret;
>
> if (var_len > LTTNG_UST_CHANNEL_DATA_MAX_LEN) {
> len = -EINVAL;
> @@ -627,9 +627,21 @@ ssize_t ustcomm_recv_channel_from_sessiond(int sock,
> goto error_recv;
> }
> }
> - *_wakeup_fd = wakeup_fd;
> - lttng_ust_add_fd_to_tracker(wakeup_fd);
> +
> + ret = lttng_ust_add_fd_to_tracker(wakeup_fd);
> + if (ret < 0) {
> + lttng_ust_unlock_fd_tracker();
> + ret = close(wakeup_fd);
> + if (ret) {
> + PERROR("close on wakeup_fd");
> + }
> + len = -EIO;
> + goto error_recv;
> + }
> +
> + *_wakeup_fd = ret;
> lttng_ust_unlock_fd_tracker();
> +
> *_chan_data = chan_data;
> return len;
>
> @@ -661,10 +673,35 @@ int ustcomm_recv_stream_from_sessiond(int sock,
> goto error;
> }
> }
> - *shm_fd = fds[0];
> - *wakeup_fd = fds[1];
> - lttng_ust_add_fd_to_tracker(fds[0]);
> - lttng_ust_add_fd_to_tracker(fds[1]);
> +
> + ret = lttng_ust_add_fd_to_tracker(fds[0]);
> + if (ret < 0) {
> + lttng_ust_unlock_fd_tracker();
> + ret = close(fds[0]);
> + if (ret) {
> + PERROR("close on received shm_fd");
> + }
> + ret = -EIO;
> + goto error;
> + }
> + *shm_fd = ret;
> +
> + ret = lttng_ust_add_fd_to_tracker(fds[1]);
> + if (ret < 0) {
> + lttng_ust_unlock_fd_tracker();
> + ret = close(*shm_fd);
> + if (ret) {
> + PERROR("close on shm_fd");
> + }
> + *shm_fd = -1;
> + ret = close(fds[1]);
> + if (ret) {
> + PERROR("close on received wakeup_fd");
> + }
> + ret = -EIO;
> + goto error;
> + }
> + *wakeup_fd = ret;
> lttng_ust_unlock_fd_tracker();
> return 0;
>
> diff --git a/liblttng-ust-comm/lttng-ust-fd-tracker.c
> b/liblttng-ust-comm/lttng-ust-fd-tracker.c
> index 8d2acb6..c2b3e3d 100644
> --- a/liblttng-ust-comm/lttng-ust-fd-tracker.c
> +++ b/liblttng-ust-comm/lttng-ust-fd-tracker.c
> @@ -47,6 +47,7 @@
> #define IS_FD_VALID(fd) ((fd) >= 0 && (fd) < lttng_ust_max_fd)
> #define GET_FD_SET_FOR_FD(fd, fd_sets) (&((fd_sets)[(fd) / FD_SETSIZE]))
> #define CALC_INDEX_TO_SET(fd) ((fd) % FD_SETSIZE)
> +#define IS_FD_STD(fd) (IS_FD_VALID(fd) && (fd) <= STDERR_FILENO)
>
> /* Check fd validity before calling these. */
> #define ADD_FD_TO_SET(fd, fd_sets) \
> @@ -143,25 +144,110 @@ void lttng_ust_unlock_fd_tracker(void)
> URCU_TLS(thread_fd_tracking) = 0;
> }
>
> +static int dup_std_fd(int fd)
> +{
> + int ret;
> + int fd_to_close[STDERR_FILENO + 1];
> + int fd_to_close_count = 0;
> + int dup_cmd = F_DUPFD; /* Default command */
> + int fd_valid = -1;
> +
> + if (!(IS_FD_STD(fd))) {
> + /* Should not be here */
> + ret = -1;
> + goto error;
> + }
> +
> + /* Check for FD_CLOEXEC flag */
> + ret = fcntl(fd, F_GETFD);
> + if (ret < 0) {
> + PERROR("fcntl on f_getfd");
> + ret = -1;
> + goto error;
> + }
> +
> + if (ret & FD_CLOEXEC) {
> + dup_cmd = F_DUPFD_CLOEXEC;
> + }
> +
> + /* Perform dup */
> + for (int i = 0; i < STDERR_FILENO + 1; i++) {
> + ret = fcntl(fd, dup_cmd, 0);
> + if (ret < 0) {
> + PERROR("fcntl dup fd");
> + goto error;
> + }
> +
> + if (!(IS_FD_STD(ret))) {
> + /* fd is outside of STD range, use it. */
> + fd_valid = ret;
> + /* Close fd received as argument. */
> + fd_to_close[i] = fd;
> + fd_to_close_count++;
> + break;
> + }
> +
> + fd_to_close[i] = ret;
> + fd_to_close_count++;
> + }
> +
> + /* Close intermediary fds */
> + for (int i = 0; i < fd_to_close_count; i++) {
> + ret = close(fd_to_close[i]);
> + if (ret) {
> + PERROR("close on temporary fd: %d.", fd_to_close[i]);
> + /*
> + * Not using an abort here would yield a complicated
> + * error handling for the caller. If a failure occurs
> + * here, the system is already in a bad state.
> + */
> + abort();
> + }
> + }
> +
> + ret = fd_valid;
> +error:
> + return ret;
> +}
> +
> /*
> * Needs to be called with ust_safe_guard_fd_mutex held when opening the fd.
> * Has strict checking of fd validity.
> + *
> + * If fd <= 2, dup the fd until fd > 2. This enable us to bypass problems that
> + * can be encounter if UST uses stdin, stdout, stderr fds for internal use
> + * (daemon etc.). Intermediary fds are closed as needed.
> + *
> + * Return -1 on error.
> + *
> */
> -void lttng_ust_add_fd_to_tracker(int fd)
> +int lttng_ust_add_fd_to_tracker(int fd)
> {
> + int ret;
> /*
> * Ensure the tracker is initialized when called from
> * constructors.
> */
> lttng_ust_init_fd_tracker();
> -
> assert(URCU_TLS(thread_fd_tracking));
> +
> + if (IS_FD_STD(fd)) {
> + ret = dup_std_fd(fd);
> + if (ret < 0) {
> + goto error;
> + }
> + fd = ret;
> + }
> +
> /* Trying to add an fd which we can not accommodate. */
> assert(IS_FD_VALID(fd));
> /* Setting an fd thats already set. */
> assert(!IS_FD_SET(fd, lttng_fd_set));
>
> ADD_FD_TO_SET(fd, lttng_fd_set);
> + return fd;
> +error:
> + return ret;
> }
>
> /*
> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
> index 511b9cf..32ba3c1 100644
> --- a/liblttng-ust/lttng-ust-comm.c
> +++ b/liblttng-ust/lttng-ust-comm.c
> @@ -1261,7 +1261,18 @@ char *get_map_shm(struct sock_info *sock_info)
> lttng_ust_unlock_fd_tracker();
> goto error;
> }
> - lttng_ust_add_fd_to_tracker(wait_shm_fd);
> +
> + ret = lttng_ust_add_fd_to_tracker(wait_shm_fd);
> + if (ret < 0) {
> + ret = close(wait_shm_fd);
> + if (!ret) {
> + PERROR("Error closing fd");
> + }
> + lttng_ust_unlock_fd_tracker();
> + goto error;
> + }
> +
> + wait_shm_fd = ret;
> lttng_ust_unlock_fd_tracker();
>
> wait_shm_mmap = mmap(NULL, page_size, PROT_READ,
> @@ -1353,7 +1364,7 @@ static
> void *ust_listener_thread(void *arg)
> {
> struct sock_info *sock_info = arg;
> - int sock, ret, prev_connect_failed = 0, has_waited = 0;
> + int sock, ret, prev_connect_failed = 0, has_waited = 0, fd;
> long timeout;
>
> lttng_ust_fixup_tls();
> @@ -1433,9 +1444,21 @@ restart:
> ust_unlock();
> goto restart;
> }
> - lttng_ust_add_fd_to_tracker(ret);
> - lttng_ust_unlock_fd_tracker();
> + fd = ret;
> + ret = lttng_ust_add_fd_to_tracker(fd);
> + if (ret < 0) {
> + ret = close(fd);
> + if (ret) {
> + PERROR("close on sock_info->socket");
> + }
> + ret = -1;
> + lttng_ust_unlock_fd_tracker();
> + ust_unlock();
> + goto quit;
> + }
> +
> sock_info->socket = ret;
> + lttng_ust_unlock_fd_tracker();
>
> ust_unlock();
> /*
> @@ -1504,9 +1527,22 @@ restart:
> ust_unlock();
> goto restart;
> }
> - lttng_ust_add_fd_to_tracker(ret);
> - lttng_ust_unlock_fd_tracker();
> +
> + fd = ret;
> + ret = lttng_ust_add_fd_to_tracker(fd);
> + if (ret < 0) {
> + ret = close(fd);
> + if (ret) {
> + PERROR("close on sock_info->notify_socket");
> + }
> + ret = -1;
> + lttng_ust_unlock_fd_tracker();
> + ust_unlock();
> + goto quit;
> + }
> +
> sock_info->notify_socket = ret;
> + lttng_ust_unlock_fd_tracker();
>
> ust_unlock();
> /*
> diff --git a/liblttng-ust/lttng-ust-elf.c b/liblttng-ust/lttng-ust-elf.c
> index f2c0982..c073e7a 100644
> --- a/liblttng-ust/lttng-ust-elf.c
> +++ b/liblttng-ust/lttng-ust-elf.c
> @@ -243,6 +243,7 @@ struct lttng_ust_elf *lttng_ust_elf_create(const char *path)
> uint8_t e_ident[EI_NIDENT];
> struct lttng_ust_elf_shdr *section_names_shdr;
> struct lttng_ust_elf *elf = NULL;
> + int ret, fd;
>
> elf = zmalloc(sizeof(struct lttng_ust_elf));
> if (!elf) {
> @@ -256,12 +257,23 @@ struct lttng_ust_elf *lttng_ust_elf_create(const char
> *path)
> }
>
> lttng_ust_lock_fd_tracker();
> - elf->fd = open(elf->path, O_RDONLY | O_CLOEXEC);
> - if (elf->fd < 0) {
> + fd = open(elf->path, O_RDONLY | O_CLOEXEC);
> + if (fd < 0) {
> lttng_ust_unlock_fd_tracker();
> goto error;
> }
> - lttng_ust_add_fd_to_tracker(elf->fd);
> +
> + ret = lttng_ust_add_fd_to_tracker(fd);
> + if (ret < 0) {
> + ret = close(fd);
> + if (ret) {
> + PERROR("close on elf->fd");
> + }
> + ret = -1;
> + lttng_ust_unlock_fd_tracker();
> + goto error;
> + }
> + elf->fd = ret;
> lttng_ust_unlock_fd_tracker();
>
> if (lttng_ust_read(elf->fd, e_ident, EI_NIDENT) < EI_NIDENT) {
> --
> 2.7.4
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list