Hi Richard,
Richard Genoud <richard.gen...@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.gen...@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.gen...@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.gen...@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.gen...@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.
Thanks,
Miquèl
