Re: [PATCH] selftests/mm/cow : Fix memory leak in child_vmsplice_memcmp_fn()

[David Hildenbrand <david@redhat.com>]

On 15.01.25 06:45, liuye wrote:
> 
> 在 2025/1/14 18:23, David Hildenbrand 写道:
>> On 14.01.25 03:29, liuye wrote:
>>>       Release memory before exception branch returns to prevent memory
>>> leaks.
>>>
>>> Signed-off-by: liuye <liuye@kylinos.cn>
>>> ---
>>>    tools/testing/selftests/mm/cow.c | 21 ++++++++++++++++-----
>>>    1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/cow.c
>>> b/tools/testing/selftests/mm/cow.c
>>> index 1238e1c5aae1..959327ba6258 100644
>>> --- a/tools/testing/selftests/mm/cow.c
>>> +++ b/tools/testing/selftests/mm/cow.c
>>> @@ -167,19 +167,30 @@ static int child_vmsplice_memcmp_fn(char *mem,
>>> size_t size,
>>>        /* Backup the original content. */
>>>        memcpy(old, mem, size);
>>>    -    if (pipe(fds) < 0)
>>> +    if (pipe(fds) < 0) {
>>> +        free(old);
>>> +        free(new);
>>>            return -errno;
>>> -
>>> +    }
>>>        /* Trigger a read-only pin. */
>>>        transferred = vmsplice(fds[1], &iov, 1, 0);
>>> -    if (transferred < 0)
>>> +    if (transferred < 0) {
>>> +        free(old);
>>> +        free(new);
>>>            return -errno;
>>> -    if (transferred == 0)
>>> +    }
>>> +    if (transferred == 0) {
>>> +        free(old);
>>> +        free(new);
>>>            return -EINVAL;
>>> +    }
>>>          /* Unmap it from our page tables. */
>>> -    if (munmap(mem, size) < 0)
>>> +    if (munmap(mem, size) < 0) {
>>> +        free(old);
>>> +        free(new);
>>>            return -errno;
>>> +    }
>>
>> We are immediately exiting the test in do_test_cow_in_parent()
>>      exit(fn(mem, size, &comm_pipes));
>>
>> Your changes make the code unnecessarily more complicated to read, so
>> I'm not in favor of this one to make some checker tool happy.
>>
> 
> It is indeed exiting the process. After the process exits, the memory
> will be reclaimed naturally.
> 
> As code in the kernel branch, it will be used by beginners, such as me,
> for learning. Modifications are recommended.
> 
> Regarding the readability of the code, is the following modification
> better than before?
> 
> 
> 
> diff --git a/tools/testing/selftests/mm/cow.c
> b/tools/testing/selftests/mm/cow.c
> index 1238e1c5aae1..db5e71c5bf2f 100644
> --- a/tools/testing/selftests/mm/cow.c
> +++ b/tools/testing/selftests/mm/cow.c
> @@ -167,19 +167,21 @@ static int child_vmsplice_memcmp_fn(char *mem,
> size_t size,
>           /* Backup the original content. */
>           memcpy(old, mem, size);
> 
> -       if (pipe(fds) < 0)
> -               return -errno;
> +       if (pipe(fds) < 0)
> +               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)
> +               goto free;
> +       if (transferred == 0) {
> +               error = EINVAL;
> +               goto free;
> +       }
> 
>           /* Unmap it from our page tables. */
>           if (munmap(mem, size) < 0)
> -               return -errno;
> +               goto free;
> 
>           /* Wait until the parent modified it. */
>           write(comm_pipes->child_ready[1], "0", 1);
> @@ -194,6 +196,10 @@ static int child_vmsplice_memcmp_fn(char *mem,
> size_t size,
>           }
> 
>           return memcmp(old, new, transferred);
> +free:
> +       free(old);
> +       free(new);
> +       return -error;
>    }

As an alternative, we can replace all return statements by explicit exit() ?

-- 
Cheers,

David / dhildenb