linux-kernel - Re: [PATCH] selftests/mm: fix memory leak in child_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 for Android: free password hash cracker in your pocket

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

Message-ID: <35c413a1-02df-21c0-99e1-2e143c945ae4@redhat.com>
Date:   Thu, 6 Apr 2023 12:37:09 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Feng Jiang <jiangfeng@...inos.cn>, akpm@...ux-foundation.org,
        shuah@...nel.org
Cc:     linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ming Xie <xieming@...inos.cn>
Subject: Re: [PATCH] selftests/mm: fix memory leak in child_memcmp_fn

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@...inos.cn>
>>> Suggested-by: Ming Xie <xieming@...inos.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