[PATCH] server: Fix shm_create_pool size fail path fd leak

[Jonas Ådahl]

On Thu, Feb 18, 2016 at 11:59:29PM +0100, Sergi Granell wrote:
> If the client passed a size <= 0 to shm_create_pool, it would
> go to err_free, which wouldn't close the fd, and thus leave it opened.
> 
> We can also move the size check before the struct wl_shm_pool
> malloc, so in case the client passes a wrong size, it won't
> do an unnecessary malloc and then free.

Did you detect this using valgrind or something like that? Because IIRC,
we collect all transmitted file descriptors and close them when the
client is destroyed (see wl_connection_destory()). Since we post an
error, the client will eventually be destroyed, so we shouldn't leak
the fd here already.


Jonas

> ---
>  src/wayland-shm.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> index a4343a4..81bf657 100644
> --- a/src/wayland-shm.c
> +++ b/src/wayland-shm.c
> @@ -230,17 +230,17 @@ shm_create_pool(struct wl_client *client, struct wl_resource *resource,
>  {
>  	struct wl_shm_pool *pool;
>  
> -	pool = malloc(sizeof *pool);
> -	if (pool == NULL) {
> -		wl_client_post_no_memory(client);
> -		goto err_close;
> -	}
> -
>  	if (size <= 0) {
>  		wl_resource_post_error(resource,
>  				       WL_SHM_ERROR_INVALID_STRIDE,
>  				       "invalid size (%d)", size);
> -		goto err_free;
> +		goto err_close;
> +	}
> +
> +	pool = malloc(sizeof *pool);
> +	if (pool == NULL) {
> +		wl_client_post_no_memory(client);
> +		goto err_close;
>  	}
>  
>  	pool->refcount = 1;
> @@ -251,7 +251,7 @@ shm_create_pool(struct wl_client *client, struct wl_resource *resource,
>  		wl_resource_post_error(resource,
>  				       WL_SHM_ERROR_INVALID_FD,
>  				       "failed mmap fd %d", fd);
> -		goto err_close;
> +		goto err_free;
>  	}
>  	close(fd);
>  
> @@ -270,10 +270,10 @@ shm_create_pool(struct wl_client *client, struct wl_resource *resource,
>  
>  	return;
>  
> -err_close:
> -	close(fd);
>  err_free:
>  	free(pool);
> +err_close:
> +	close(fd);
>  }
>  
>  static const struct wl_shm_interface shm_interface = {
> -- 
> 2.7.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel