From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Tue, 22 Dec 2020 08:46:17 +0100 Subject: [PATCH] fs: squasfs: fix a possible NULL pointer dereference in sqfs_opendir() In-Reply-To: <3b40a215-ffff-80f2-39bd-88c5812cf2a0@posteo.net> References: <20201218142440.21783-1-richard.genoud@posteo.net> <20201218195009.056da9d8@xps13> <20201221161419.478626f2@xps13> <20201221162919.2133c37f@xps13> <8a09797d-4f9d-42cb-ea25-99e9c7c85376@posteo.net> <20201221164954.0dc53f15@xps13> <3b40a215-ffff-80f2-39bd-88c5812cf2a0@posteo.net> Message-ID: <20201222084617.162d4a80@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Richard, Richard Genoud 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 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 wrote on Mon, 21 Dec 2020 > >>> 16:26:00 +0100: > >>> >>>> Le 21/12/2020 ? 16:14, Miquel Raynal a ?crit?: > >>>>> Hi Richard, > >>>>> > >>>>> Richard Genoud 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 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