Re: [PATCH] selftests/mm: cow: fix leaks in child_vmsplice_memcmp_fn()

[Mike Rapoport <rppt@kernel.org>]

On Thu, Jul 02, 2026 at 03:24:36PM +0800, Jiangshan Yi wrote:
> child_vmsplice_memcmp_fn() allocated two heap buffers (old, new) and
> opened a pipe, but every return path - including the normal one -
> returned directly without freeing the buffers or closing the pipe file
> descriptors. It also used the malloc() results without checking them,
> so memcpy(old, mem, size) would crash on allocation failure (size can
> be as large as a huge page).
> 
> The sibling helper do_test_vmsplice_in_parent() in the same file already
> uses the goto-based cleanup pattern; follow it here.

The difference is that child_vmsplice_memcmp_fn() runs in a forked process
that dies if any error happens, so the allocated resources are anyway
immediately freed.

> Check the allocations up front

This is the only check that is actually required, others are nice to have
at best.

I'd keep the code simple and only add the allocation check that returns
-ENOMEM on failure.

> and introduce unified error handling with goto labels so that old/new are
> always freed and both pipe fds are always closed before returning, on
> every path.
> 
> Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>
> ---
>  tools/testing/selftests/mm/cow.c | 44 ++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
> index 0c627ea89ff7..f6a1bedfd6e2 100644
> --- a/tools/testing/selftests/mm/cow.c
> +++ b/tools/testing/selftests/mm/cow.c
> @@ -145,26 +145,39 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size,
>  	char *old, *new;
>  	int fds[2];
>  	char buf;
> +	int ret;
>  
>  	old = malloc(size);
>  	new = malloc(size);
> +	if (!old || !new) {
> +		ret = -ENOMEM;
> +		goto free;
> +	}
>  
>  	/* Backup the original content. */
>  	memcpy(old, mem, size);
>  
> -	if (pipe(fds) < 0)
> -		return -errno;
> +	if (pipe(fds) < 0) {
> +		ret = -errno;
> +		goto free;
> +	}
>  
>  	/* Trigger a read-only pin. */
>  	transferred = vmsplice(fds[1], &iov, 1, 0);
> -	if (transferred < 0)
> -		return -errno;
> -	if (transferred == 0)
> -		return -EINVAL;
> +	if (transferred < 0) {
> +		ret = -errno;
> +		goto close_pipe;
> +	}
> +	if (transferred == 0) {
> +		ret = -EINVAL;
> +		goto close_pipe;
> +	}
>  
>  	/* Unmap it from our page tables. */
> -	if (munmap(mem, size) < 0)
> -		return -errno;
> +	if (munmap(mem, size) < 0) {
> +		ret = -errno;
> +		goto close_pipe;
> +	}
>  
>  	/* Wait until the parent modified it. */
>  	write(comm_pipes->child_ready[1], "0", 1);
> @@ -174,11 +187,20 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size,
>  	/* See if we still read the old values via the pipe. */
>  	for (total = 0; total < transferred; total += cur) {
>  		cur = read(fds[0], new + total, transferred - total);
> -		if (cur < 0)
> -			return -errno;
> +		if (cur < 0) {
> +			ret = -errno;
> +			goto close_pipe;
> +		}
>  	}
>  
> -	return memcmp(old, new, transferred);
> +	ret = memcmp(old, new, transferred);
> +close_pipe:
> +	close(fds[0]);
> +	close(fds[1]);
> +free:
> +	free(old);
> +	free(new);
> +	return ret;
>  }
>  
>  typedef int (*child_fn)(char *mem, size_t size, struct comm_pipes *comm_pipes);
> -- 
> 2.25.1
> 

-- 
Sincerely yours,
Mike.