[PATCH] fs: squasfs: fix a possible NULL pointer dereference in sqfs_opendir()

[Richard Genoud]

Hi Miquel
Le 22/12/2020 à 08:46, Miquel Raynal a écrit :
> Hi Richard,
> 
> Richard Genoud <richard.genoud at posteo.net> wrote on Mon, 21 Dec 2020
> 17:17:56 +0100:
> 
>> Hi Miquel
>>
>> Le 21/12/2020 à 16:49, Miquel Raynal a écrit :
>>> Hi Richard,
>>>
>>> Richard Genoud <richard.genoud at posteo.net> wrote on Mon, 21 Dec 2020
>>> 16:40:51 +0100:
>>>    
>>>> Hi Miquel,
>>>>
>>>> Le 21/12/2020 à 16:29, Miquel Raynal a écrit :
>>>>> Hi Richard,
>>>>>
>>>>> Richard Genoud <richard.genoud at posteo.net> wrote on Mon, 21 Dec 2020
>>>>> 16:26:00 +0100:
>>>>>     >>>> Le 21/12/2020 à 16:14, Miquel Raynal a écrit :
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>> Richard Genoud <richard.genoud at posteo.net> wrote on Mon, 21 Dec 2020
>>>>>>> 16:06:37 +0100:
>>>>>>>      >>>> Hi Miquel,
>>>>>>>>
>>>>>>>> Le 18/12/2020 à 19:50, Miquel Raynal a écrit :
>>>>>>>>> Hi Richard,
>>>>>>>>>
>>>>>>>>> Richard Genoud <richard.genoud at posteo.net> wrote on Fri, 18 Dec 2020
>>>>>>>>> 15:24:40 +0100:
>>>>>>>>>       >>>> token_count may be != 0 and token_list not yet allocated when the out
>>>>>>>>>> code is reached
>>>>>>>>>
>>>>>>>>> Wouldn't it be better to initialize token_count than adding an
>>>>>>>>> (obscure) indentation level?
>>>>>>>> Well, token_count is initialized :
>>>>>>>> token_count = sqfs_count_tokens(filename);
>>>>>>>>
>>>>>>>> But token_list is not yet populated. It is some lines bellow:
>>>>>>>> token_list = malloc(token_count * sizeof(char *));
>>>>>>>>
>>>>>>>>
>>>>>>>> But I could use something like that, maybe it's clearer :
>>>>>>>> 	for (j = 0; (token_list != NULL) && (j < token_count); j++)
>>>>>>>> 		free(token_list[j]);
>>>>>>>
>>>>>>> I had a look at the code, the error path is clearly not correctly
>>>>>>> organized.
>>>>>>>
>>>>>>> I think the right approach would be to have real labels like,
>>>>>>> free_token_list, free_this, free_that and for each of them only do the
>>>>>>> right cleanup. Doing so should fix the issue.
>>>>>>
>>>>>> So you're suggesting to revert this ?
>>>>>> commit ea1b1651c6a8 ("fs/squashfs: sqfs_opendir: simplify error handling")
>>>>>
>>>>> Yes (our e-mails crossed each other), I think it's best to have a well
>>>>> organized error path. Of course this error path is maybe faulty, in
>>>>> this case it must be fixed. But I personally prefer the revert + fix
>>>>> approach.
>>>>>     >>
>>>> But I really don't see why it's obscure to test a pointer before dereference.
>>>
>>> Testing a pointer before dereference is not obscure.
>>>
>>> Testing a pointer in an error path because the error path is common to
>>> all 10 different possible failure cases and might free the content of an
>>> array that has not been allocated yet: this is obscure.
>>>    
>>>> Maybe I should I've wrote :
>>>>           if (token_list != NULL)
>>>>                   for (j = 0; j < token_count; j++)
>>>>                           free(token_list[j]);
>>>>
>>>> Does it looks better ?
>>>
>>> Not really :)
>>
>> Ok, so if you insist, I send the revert correcting the coverity issue.
>>
>> But in this case, the error management won't be coherent with the rest of the file.
>> (And I *really* don't want to revert to the old error handling for every single function.)
> 
> Well, I was against taking this direction from the beginning, now we
> are at a point where the error path must be fixed because you need to
> take extra precautions that you would have avoided by keeping the well
> organized labels.
Ok, let's agree to disagree.
You have a strong opinion on your way to do things, and so do I.

No need to give me the "I told you so" card here, because it won't help in anyway.

> 
> Thanks,
> Miquèl
> 
Regards,
Richard