linux-kernel - Re: [PATCH] selftests/mm/cow : Fix memory leak in child_vmsplice_memcmp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3613bdb9-a933-4027-89e9-03a261410f68@redhat.com>
Date: Mon, 20 Jan 2025 21:16:50 +0100
From: David Hildenbrand <david@...hat.com>
To: liuye <liuye@...inos.cn>, akpm@...ux-foundation.org, shuah@...nel.org
Cc: linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests/mm/cow : Fix memory leak in
 child_vmsplice_memcmp_fn()

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@...inos.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