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

[Richard Genoud]

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

> Thanks,
> Miquèl
> 
Thanks!