From fc9a0ccbfb0e968fde8cbb27f19852e0571adedc Mon Sep 17 00:00:00 2001 From: Jonathan Rajotte Date: Mon, 5 Feb 2018 17:58:18 -0500 Subject: [PATCH] Force tracked fd to be bigger than STDERR_FILENO 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 Signed-off-by: Mathieu Desnoyers --- include/ust-fd.h | 2 +- liblttng-ust-comm/lttng-ust-comm.c | 51 +++++++++++-- liblttng-ust-comm/lttng-ust-fd-tracker.c | 92 +++++++++++++++++++++++- liblttng-ust/lttng-ust-comm.c | 48 +++++++++++-- liblttng-ust/lttng-ust-elf.c | 18 ++++- 5 files changed, 192 insertions(+), 19 deletions(-) diff --git a/include/ust-fd.h b/include/ust-fd.h index 032cac53..a0155251 100644 --- a/include/ust-fd.h +++ b/include/ust-fd.h @@ -27,7 +27,7 @@ #include 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 9fe6d288..3d1c6503 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 8d2acb69..329339f9 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,112 @@ 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 enables us to bypass + * problems that can be encountered if UST uses stdin, stdout, stderr + * fds for internal use (daemon etc.). This can happen if the + * application closes either of those file descriptors. 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 776ad332..e92bb870 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -1247,7 +1247,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, @@ -1339,7 +1350,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(); @@ -1419,9 +1430,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(); /* @@ -1490,9 +1513,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 f2c09829..c073e7a5 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.39.5