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: <778941db-27fb-48b3-8d98-81d1673deffc@kylinos.cn>
Date: Wed, 15 Jan 2025 13:45:09 +0800
From: liuye <liuye@...inos.cn>
To: David Hildenbrand <david@...hat.com>, 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()


在 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;
  }