[U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference

[Heiko Schocher]

Hello Ladislav,

Sorry for digging into it so late...

Am 22.11.2017 um 09:25 schrieb Ladislav Michl:
> On Wed, Nov 22, 2017 at 09:09:36AM +0100, Wolfgang Denk wrote:
>> Dear Ladislav,
>>
>> In message <20171121212222.ryicwv6tyh5rye2e at lenoch> you wrote:
>>>>>
>>>>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>>>>> index 4465523d5f..313dee0579 100644
>>>>> --- a/fs/ubifs/ubifs.c
>>>>> +++ b/fs/ubifs/ubifs.c
>>>>> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>>>>   	dir = kzalloc(sizeof(struct inode), 0);
>>>>>   	if (!file || !dentry || !dir) {
>>>>>   		printf("%s: Error, no memory for malloc!\n", __func__);
>>>>> -		err = -ENOMEM;
>>>>> -		goto out;
>>>>> +		goto out_nomem;
>>>>>   	}
>> ...
>>>> Should you not keep the "err = -ENOMEM;" setting?  Otherwise there
>>>> is no indivcation that an error happened.
>>>
>>> It is not obvious from the patch, but value of err is later discarded.
>>> It serves sole purpose of printing debug notice.
>>
>> So apparently we have a number of places in U-Boot where fatal
>> errors (running out of memory) are just ignored and we continue as
>> if nothing happened?
> 
> While I have to admit this code is not an example of clean coding,
> it prints notice when trying to manipulate with file.
> 
> fs/ubifs/ubifs.c as whole needs to be revisited, above patch just
> caused shit hitting the fan.

Yes, full ack ... we do need here a full review of this code.

>> THis is short-sighted at best. One day Pump Six will fail.
>>
>> This is giving me the creepes.
> 
> I was just pointing to the fact, that above mentioned patch does not
> make it any worse. Btw, initial commit is even more amazing:
> 
> +       if (file)
> +               free(file);
> +       if (dentry)
> +               free(dentry);
> +       if (dir)
> +               free(dir);
> +
> +       if (file->private_data)
> +               kfree(file->private_data);
> +       file->private_data = NULL;
> +       file->f_pos = 2;
> +       return 0;
> 
> .oO(I guess it is safe not pointing where above snippet is comming from,
> but will review whole file as I'm going to use it for new board I have to
> suport)

Many thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de