Re: [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.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.
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!