[lttng-dev] [PATCH lttng-ust] Fix: fd of an elf object must be registered to the fd tracker

[Mathieu Desnoyers]

----- On Nov 9, 2017, at 5:36 PM, Jonathan Rajotte jonathan.rajotte-julien at efficios.com wrote:

> The open call take place inside ust, it must be tracked to prevent external
> closing.
> 
> The bug can be hit during tracing of an application for which the probe
> provider is loaded using LD_PRELOAD in combination with the fd utility
> shared object. The application is responsible for closing all possible fd.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> ---
> liblttng-ust/lttng-ust-elf.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/liblttng-ust/lttng-ust-elf.c b/liblttng-ust/lttng-ust-elf.c
> index a496841a..5f3b280e 100644
> --- a/liblttng-ust/lttng-ust-elf.c
> +++ b/liblttng-ust/lttng-ust-elf.c
> @@ -27,6 +27,7 @@
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdbool.h>
> +#include <ust-fd.h>
> #include "lttng-tracer-core.h"
> 
> #define BUF_LEN	4096
> @@ -242,6 +243,8 @@ 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 fd;
> +	int ret;
> 
> 	elf = zmalloc(sizeof(struct lttng_ust_elf));
> 	if (!elf) {
> @@ -253,10 +256,16 @@ struct lttng_ust_elf *lttng_ust_elf_create(const char
> *path)
> 		goto error;
> 	}
> 
> -	elf->fd = open(elf->path, O_RDONLY | O_CLOEXEC);
> -	if (elf->fd < 0) {
> +	lttng_ust_lock_fd_tracker();
> +	fd = open(elf->path, O_RDONLY | O_CLOEXEC);
> +	if (fd < 0) {
> +		lttng_ust_unlock_fd_tracker();
> 		goto error;
> 	}
> +	lttng_ust_add_fd_to_tracker(fd);
> +	lttng_ust_unlock_fd_tracker();
> +
> +	elf->fd = fd;
> 
> 	if (lttng_ust_read(elf->fd, e_ident, EI_NIDENT) < EI_NIDENT) {
> 		goto error;
> @@ -312,9 +321,15 @@ error:
> 	if (elf) {
> 		free(elf->ehdr);
> 		if (elf->fd >= 0) {
> -			if (close(elf->fd)) {
> +			lttng_ust_lock_fd_tracker();
> +			ret = close(elf->fd);
> +			if (!ret) {
> +				lttng_ust_delete_fd_from_tracker(elf->fd);
> +			} else {
> +				PERROR("close");
> 				abort();
> 			}
> +			lttng_ust_lock_fd_tracker();

This appears to be a double-lock.

How did you test this code path ?

Thanks,

Mathieu


> 		}
> 		free(elf->path);
> 		free(elf);
> @@ -339,14 +354,23 @@ uint8_t lttng_ust_elf_is_pic(struct lttng_ust_elf *elf)
>  */
> void lttng_ust_elf_destroy(struct lttng_ust_elf *elf)
> {
> +	int ret;
> +
> 	if (!elf) {
> 		return;
> 	}
> 
> -	free(elf->ehdr);
> -	if (close(elf->fd)) {
> +	lttng_ust_lock_fd_tracker();
> +	ret = close(elf->fd);
> +	if (!ret) {
> +		lttng_ust_delete_fd_from_tracker(elf->fd);
> +	} else {
> +		PERROR("close");
> 		abort();
> 	}
> +	lttng_ust_unlock_fd_tracker();
> +
> +	free(elf->ehdr);
> 	free(elf->path);
> 	free(elf);
> }
> --
> 2.11.0

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com