linux-ext4 - Re: [PATCH v2 05/12] ss_create_invocation: fix memory leak and check whether NULL pointer

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: <63560c80-4519-7cee-f948-4e909f5a2e05@huawei.com>
Date:   Mon, 19 Jul 2021 19:08:04 +0800
From:   Zhiqiang Liu <liuzhiqiang26@...wei.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>,
        wuguanghao <wuguanghao3@...wei.com>
CC:     <linux-ext4@...r.kernel.org>, <artem.blagodarenko@...il.com>,
        <linfeilong@...wei.com>
Subject: Re: [PATCH v2 05/12] ss_create_invocation: fix memory leak and check
 whether NULL pointer


On 2021/7/16 11:34, Theodore Y. Ts'o wrote:
> On Wed, Jun 30, 2021 at 04:27:17PM +0800, wuguanghao wrote:
>> In ss_create_invocation(), it is necessary to check whether
>> returned by malloc is a null pointer.
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@...wei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@...wei.com>
>> ---
>>  lib/ss/invocation.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
> Instead of adding all of these goto targets (which is fragile if for
> some reason the code gets rearranged), it would be better to make sure
> everything that we might want to free is initialized, i.e.:
>
>   	register ss_data *new_table = NULL;
>   	register ss_data **table = NULL;
>
>   	new_table = (ss_data *) malloc(sizeof(ss_data));
> 	if (!new_table)
> 		goto out;
> 	memset(new_table, 0, sizeof(ss_data));
>
> and then exit path can just look like this:
>
> out:
> 	if (new_table) {
> 		free(new_table->prompt);
> 		free(new_table->info_dirs);
> 	}
> 	free(new_table);
> 	free(table);
> 	*code_ptr = ENOMEM;
> 	return 0;
>
> ... which is much cleaner, and means in the future, you don't need to
> figure out which goto label you might need to jump to.
>
> Cheers,
>
> 					- Ted
>
> P.S.  And if we are making all of these changes to the function's
> initializers, it might be a good time to zap the "register" keywords
> for any lines we are changing, or are nearby, while we're at it.
>
> .

Thanks for your suggestion.

We will rewrite the patch and remove the "register" keywords.


Regards

Zhiqiang Liu

.