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

[Miquel Raynal]

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


Thanks,
Miquèl