| Message ID | 20201218142440.21783-1-richard.genoud@posteo.net |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs: squasfs: fix a possible NULL pointer dereference in sqfs_opendir() | expand |
Hi Richard, Richard Genoud <richard.genoud@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? > > Reported-by: Coverity CID 313547 > Fixes: ea1b1651c6a8 ("fs/squashfs: sqfs_opendir: simplify error handling") > Signed-off-by: Richard Genoud <richard.genoud@posteo.net> > --- > fs/squashfs/sqfs.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > index 608a2bb454c..c47046b76e5 100644 > --- a/fs/squashfs/sqfs.c > +++ b/fs/squashfs/sqfs.c > @@ -949,8 +949,9 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) > *dirsp = (struct fs_dir_stream *)dirs; > > out: > - for (j = 0; j < token_count; j++) > - free(token_list[j]); > + if (token_list) > + for (j = 0; j < token_count; j++) > + free(token_list[j]); > free(token_list); > free(pos_list); > free(path); Thanks, Miquèl
Hi Miquel, Le 18/12/2020 à 19:50, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@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]); > >> >> Reported-by: Coverity CID 313547 >> Fixes: ea1b1651c6a8 ("fs/squashfs: sqfs_opendir: simplify error handling") >> Signed-off-by: Richard Genoud <richard.genoud@posteo.net> >> --- >> fs/squashfs/sqfs.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c >> index 608a2bb454c..c47046b76e5 100644 >> --- a/fs/squashfs/sqfs.c >> +++ b/fs/squashfs/sqfs.c >> @@ -949,8 +949,9 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) >> *dirsp = (struct fs_dir_stream *)dirs; >> >> out: >> - for (j = 0; j < token_count; j++) >> - free(token_list[j]); >> + if (token_list) >> + for (j = 0; j < token_count; j++) >> + free(token_list[j]); >> free(token_list); >> free(pos_list); >> free(path); > > Thanks, > Miquèl > Thanks Richard.
Hi Richard, Richard Genoud <richard.genoud@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@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. Thanks, Miquèl
Le 21/12/2020 à 16:14, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@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@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") > > Thanks, > Miquèl >
Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 21 Dec 2020 16:14:19 +0100: > Hi Richard, > > Richard Genoud <richard.genoud@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@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. Actually I remember now: I disliked your proposal of changing all named labels to a single (and quite unclear) "goto out". This is an example of why single labels should not be used IMHO. Thanks, Miquèl
Hi Richard, Richard Genoud <richard.genoud@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@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@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. Thanks, Miquèl
Hi Miquel, Le 21/12/2020 à 16:29, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@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@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@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!
Hi Richard, Richard Genoud <richard.genoud@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@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@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@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
Hi Miquel Le 21/12/2020 à 16:49, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@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@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@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@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.
Hi Richard, Richard Genoud <richard.genoud@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@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@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@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@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
Hi Miquel Le 22/12/2020 à 08:46, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@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@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@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@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@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
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 608a2bb454c..c47046b76e5 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -949,8 +949,9 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) *dirsp = (struct fs_dir_stream *)dirs; out: - for (j = 0; j < token_count; j++) - free(token_list[j]); + if (token_list) + for (j = 0; j < token_count; j++) + free(token_list[j]); free(token_list); free(pos_list); free(path);
token_count may be != 0 and token_list not yet allocated when the out code is reached Reported-by: Coverity CID 313547 Fixes: ea1b1651c6a8 ("fs/squashfs: sqfs_opendir: simplify error handling") Signed-off-by: Richard Genoud <richard.genoud@posteo.net> --- fs/squashfs/sqfs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)