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

[Richard Genoud]

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.)


Richard.