Re: [PATCH v3] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

[Ingo Molnar]

* Malaya Kumar Rout <malayarout91@xxxxxxxxx> wrote:

> Exception branch returns without closing
> the file descriptors 'file_fd' and 'fd'
> 
> Signed-off-by: Malaya Kumar Rout <malayarout91@xxxxxxxxx>
> ---
>  tools/testing/selftests/x86/lam.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
> index 18d736640ece..88482d8112de 100644
> --- a/tools/testing/selftests/x86/lam.c
> +++ b/tools/testing/selftests/x86/lam.c
> @@ -682,7 +682,7 @@ int do_uring(unsigned long lam)
>  		return 1;
>  
>  	if (fstat(file_fd, &st) < 0)
> -		return 1;
> +		goto cleanup;
>  
>  	off_t file_sz = st.st_size;
>  
> @@ -690,7 +690,7 @@ int do_uring(unsigned long lam)
>  
>  	fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks);
>  	if (!fi)
> -		return 1;
> +		goto cleanup;
>  
>  	fi->file_sz = file_sz;
>  	fi->file_fd = file_fd;
> @@ -698,7 +698,7 @@ int do_uring(unsigned long lam)
>  	ring = malloc(sizeof(*ring));
>  	if (!ring) {
>  		free(fi);
> -		return 1;
> +		goto cleanup;
>  	}
>  
>  	memset(ring, 0, sizeof(struct io_ring));
> @@ -729,6 +729,8 @@ int do_uring(unsigned long lam)
>  	}
>  
>  	free(fi);
> +cleanup:
> +	close(file_fd);
>  
>  	return ret;
>  }
> @@ -1192,6 +1194,7 @@ void *allocate_dsa_pasid(void)
>  	if (wq == MAP_FAILED)
>  		perror("mmap");
>  
> +	close(fd);
>  	return wq;

So in your previous patch you closed the file before the perror(), 
presumably so that file-leak detection in Valgrind or whatever tool you 
are using doesn't trigger.

But here it's done after the perror() call, why? It's perfectly fine to 
close the mapping fd straight after an mmap() call.

Finally, it would be nice to quote the before/after output of the leak 
detection tool you are using.

Thanks,

	Ingo