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

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

[liuye]

    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;
+	}
 
 	/* Wait until the parent modified it. */
 	write(comm_pipes->child_ready[1], "0", 1);
-- 
2.25.1

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

[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.

-- 
Cheers,

David / dhildenb

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

[liuye]

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

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

[David Hildenbrand]

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