Re: [Outreachy kernel] Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak

[Johan Hovold <johan@kernel.org>]

On Wed, Feb 22, 2017 at 03:45:43PM +0530, Gargi Sharma wrote:
> On Wed, Feb 22, 2017 at 1:20 AM, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Wed, Feb 22, 2017 at 12:36:23AM +0530, Gargi Sharma wrote:
> > > Change array index from the loop bound variable to loop index.
> > > Memory leak was occuring whenever there was more than one
> > > loopback device. The pointer to dirent structures must be
> > > individually freed before freeing the pointer array.
> > >
> > > Coccinelle Script:
> > > @@
> > > expression arr,ex1,ex2;
> > > @@
> > >
> > > for(ex1 = 0; ex1 < ex2; ex1++) { <...
> > >   arr[
> > > - ex2
> > > + ex1
> > >   ]
> > >   ...> }
> > >
> > > Signed-off-by: Gargi Sharma <gs051095@gmail.com>
> > > ---
> > > Changes in v2:
> > >       -Made the commit message clearer.
> > > ---
> > >  drivers/staging/greybus/tools/loopback_test.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/greybus/tools/loopback_test.c
> b/drivers/staging/greybus/tools/loopback_test.c
> > > index 18d7a3d..1c01833 100644
> > > --- a/drivers/staging/greybus/tools/loopback_test.c
> > > +++ b/drivers/staging/greybus/tools/loopback_test.c
> > > @@ -636,7 +636,7 @@ int find_loopback_devices(struct loopback_test *t)
> > >       ret = 0;
> > >  done:
> > >       for (i = 0; i < n; i++)
> > > -             free(namelist[n]);
> > > +             free(namelist[i]);
> > >       free(namelist);
> > >  baddir:
> > >       return ret;

> > Oh, there's an illegal free here, which probably was what Julia was
> > referring to. Yeah, not sure why things haven't blown up, perhaps there
> > happens to be NULL pointer after?
> >
> > So we are in fact always leaking the device name here.
> 
> Um, should I assign null pointers to device names before freeing them? That
> should fix illegal free I believe.

No, your fix is fine as is, it's just a matter of describing the problem
accurately in the commit message.

> > No need to resend, unless you prefer. You can add my Reviewed-by tag if
> > so.
> Not sure what this means. I can send a v3 with the above suggested changes
> if they are correct.

Sorry if I was being unclear. I meant to say that your current commit
message is good enough, but perhaps it is indeed better if you do send a
v3 where you mention that we have been leaking all the device names
always (and not just when there have been more than one as I incorrectly
suggested).

You can also mention that you fix that illegal free of namelist[n],
which must so far have happened to be NULL (I verified that it is indeed
NULL here on my system too).

Thanks,
Johan