* [PATCH] selftests/mm: fix memory leak in child_memcmp_fn
@ 2023-04-04 3:12 Feng Jiang
2023-04-04 7:31 ` David Hildenbrand
0 siblings, 1 reply; 4+ messages in thread
From: Feng Jiang @ 2023-04-04 3:12 UTC (permalink / raw
To: akpm, shuah; +Cc: linux-mm, linux-kselftest, linux-kernel, Feng Jiang, Ming Xie
The allocated memory should be freed on return.
Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn>
Suggested-by: Ming Xie <xieming@kylinos.cn>
---
tools/testing/selftests/mm/cow.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
index 0eb2e8180aa5..c0dd2dfca51b 100644
--- a/tools/testing/selftests/mm/cow.c
+++ b/tools/testing/selftests/mm/cow.c
@@ -162,6 +162,10 @@ static int child_memcmp_fn(char *mem, size_t size,
{
char *old = malloc(size);
char buf;
+ int ret;
+
+ if (!old)
+ return -ENOMEM;
/* Backup the original content. */
memcpy(old, mem, size);
@@ -172,7 +176,10 @@ static int child_memcmp_fn(char *mem, size_t size,
;
/* See if we still read the old values. */
- return memcmp(old, mem, size);
+ ret = memcmp(old, mem, size);
+ free(old);
+
+ return ret;
}
static int child_vmsplice_memcmp_fn(char *mem, size_t size,
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/mm: fix memory leak in child_memcmp_fn
2023-04-04 3:12 [PATCH] selftests/mm: fix memory leak in child_memcmp_fn Feng Jiang
@ 2023-04-04 7:31 ` David Hildenbrand
2023-04-06 2:01 ` Feng Jiang
0 siblings, 1 reply; 4+ messages in thread
From: David Hildenbrand @ 2023-04-04 7:31 UTC (permalink / raw
To: Feng Jiang, akpm, shuah; +Cc: linux-mm, linux-kselftest, linux-kernel, Ming Xie
On 04.04.23 05:12, Feng Jiang wrote:
> The allocated memory should be freed on return.
>
> Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn>
> Suggested-by: Ming Xie <xieming@kylinos.cn>
> ---
> tools/testing/selftests/mm/cow.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
> index 0eb2e8180aa5..c0dd2dfca51b 100644
> --- a/tools/testing/selftests/mm/cow.c
> +++ b/tools/testing/selftests/mm/cow.c
> @@ -162,6 +162,10 @@ static int child_memcmp_fn(char *mem, size_t size,
> {
> char *old = malloc(size);
> char buf;
> + int ret;
> +
> + if (!old)
> + return -ENOMEM;
>
> /* Backup the original content. */
> memcpy(old, mem, size);
> @@ -172,7 +176,10 @@ static int child_memcmp_fn(char *mem, size_t size,
> ;
>
> /* See if we still read the old values. */
> - return memcmp(old, mem, size);
> + ret = memcmp(old, mem, size);
> + free(old);
> +
> + return ret;
> }
>
> static int child_vmsplice_memcmp_fn(char *mem, size_t size,
NAK, the whole point of this function is that the child process will
exit immediately after executing this function, cleaning up automatically.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/mm: fix memory leak in child_memcmp_fn
2023-04-04 7:31 ` David Hildenbrand
@ 2023-04-06 2:01 ` Feng Jiang
2023-04-06 10:37 ` David Hildenbrand
0 siblings, 1 reply; 4+ messages in thread
From: Feng Jiang @ 2023-04-06 2:01 UTC (permalink / raw
To: David Hildenbrand, akpm, shuah
Cc: linux-mm, linux-kselftest, linux-kernel, Ming Xie
On 2023/4/4 15:31, David Hildenbrand wrote:
> On 04.04.23 05:12, Feng Jiang wrote:
>> The allocated memory should be freed on return.
>>
>> Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn>
>> Suggested-by: Ming Xie <xieming@kylinos.cn>
>> ---
>> tools/testing/selftests/mm/cow.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/cow.c
>> b/tools/testing/selftests/mm/cow.c
>> index 0eb2e8180aa5..c0dd2dfca51b 100644
>> --- a/tools/testing/selftests/mm/cow.c
>> +++ b/tools/testing/selftests/mm/cow.c
>> @@ -162,6 +162,10 @@ static int child_memcmp_fn(char *mem, size_t size,
>> {
>> char *old = malloc(size);
>> char buf;
>> + int ret;
>> +
>> + if (!old)
>> + return -ENOMEM;
>> /* Backup the original content. */
>> memcpy(old, mem, size);
>> @@ -172,7 +176,10 @@ static int child_memcmp_fn(char *mem, size_t size,
>> ;
>> /* See if we still read the old values. */
>> - return memcmp(old, mem, size);
>> + ret = memcmp(old, mem, size);
>> + free(old);
>> +
>> + return ret;
>> }
>> static int child_vmsplice_memcmp_fn(char *mem, size_t size,
>
> NAK, the whole point of this function is that the child process will
> exit immediately after executing this function, cleaning up
> automatically.
>
Hi David, thanks very much for your review and I think you are right.
While the OS provides a cleanup mechanism to underwrite this, I think it
makes sense for the application to ensure that its own logic is complete
and correct.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/mm: fix memory leak in child_memcmp_fn
2023-04-06 2:01 ` Feng Jiang
@ 2023-04-06 10:37 ` David Hildenbrand
0 siblings, 0 replies; 4+ messages in thread
From: David Hildenbrand @ 2023-04-06 10:37 UTC (permalink / raw
To: Feng Jiang, akpm, shuah; +Cc: linux-mm, linux-kselftest, linux-kernel, Ming Xie
On 06.04.23 04:01, Feng Jiang wrote:
> On 2023/4/4 15:31, David Hildenbrand wrote:
>> On 04.04.23 05:12, Feng Jiang wrote:
>>> The allocated memory should be freed on return.
>>>
>>> Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn>
>>> Suggested-by: Ming Xie <xieming@kylinos.cn>
>>> ---
>>> tools/testing/selftests/mm/cow.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/cow.c
>>> b/tools/testing/selftests/mm/cow.c
>>> index 0eb2e8180aa5..c0dd2dfca51b 100644
>>> --- a/tools/testing/selftests/mm/cow.c
>>> +++ b/tools/testing/selftests/mm/cow.c
>>> @@ -162,6 +162,10 @@ static int child_memcmp_fn(char *mem, size_t size,
>>> {
>>> char *old = malloc(size);
>>> char buf;
>>> + int ret;
>>> +
>>> + if (!old)
>>> + return -ENOMEM;
>>> /* Backup the original content. */
>>> memcpy(old, mem, size);
>>> @@ -172,7 +176,10 @@ static int child_memcmp_fn(char *mem, size_t size,
>>> ;
>>> /* See if we still read the old values. */
>>> - return memcmp(old, mem, size);
>>> + ret = memcmp(old, mem, size);
>>> + free(old);
>>> +
>>> + return ret;
>>> }
>>> static int child_vmsplice_memcmp_fn(char *mem, size_t size,
>>
>> NAK, the whole point of this function is that the child process will
>> exit immediately after executing this function, cleaning up
>> automatically.
>>
> Hi David, thanks very much for your review and I think you are right.
>
> While the OS provides a cleanup mechanism to underwrite this, I think it
> makes sense for the application to ensure that its own logic is complete
> and correct.
Maybe on a per-testcase basis (which we do), such that each testcase
leaves the process in a clean slate for the next test case. But not for
such simplistic things where an exit() immediately follows. If we'd want
to cleanup any state before we call exit(), we'd end up in a lot of
unnecessarily over-complicating the code.
Just look at how many exit()/ksft_exit_fail_msg()/... we have in our
test cases. Cleaning up in these cases would be close to madness ;)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-04-06 10:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-04 3:12 [PATCH] selftests/mm: fix memory leak in child_memcmp_fn Feng Jiang
2023-04-04 7:31 ` David Hildenbrand
2023-04-06 2:01 ` Feng Jiang
2023-04-06 10:37 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.