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

Ingo Molnar
2025-04-08T08:10:04Z

* Malaya Kumar Rout <malayarout91@gmail.com> wrote:

> Exception branch returns without closing
> the file descriptors 'file_fd' and 'fd'
>
> Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.com>
> ---
> 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