Re: [Outreachy kernel] [PATCH] staging: greybus: loop variable to loop index

[Johan Hovold <johan@kernel.org>]

On Tue, Feb 21, 2017 at 08:50:33PM +0100, Julia Lawall wrote:
> On Tue, 21 Feb 2017, Johan Hovold wrote:
> > On Tue, Feb 21, 2017 at 06:31:28PM +0100, Julia Lawall wrote:
> > > On Tue, 21 Feb 2017, Gargi Sharma wrote:
> > >
> > > > Change array index from the loop bound variable to loop index.
> > > > Once all the loopback devices have been found, free the pointer
> > > > array containing the dirent structures. 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
> > > >   ]
> > > >   ...> }
> >
> > Another good catch, but you should change the patch summary to something
> > more descriptive (and less detailed) like:
> >
> > 	staging: greybus: loopback_test: fix device-name leak
> >
> > and possibly also make it more clear from the commit message that we've
> > been leaking these strings whenever there have been more than one
> > loopback device. (The memory is returned once the test program exits,
> > but still).
> >
> > Care to send a v2 with those changes?
> >
> > > > Signed-off-by: Gargi Sharma <gs051095@gmail.com>
> > > > ---
> > > >  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]);
> > >
> > > I guess this is a test, so no one ever tried running it :)
> >
> > It is a test tool, but we've been using it quite a bit.
> 
> I guess I don't know what n is.  I would have expected that it would be an
> out of bounds array reference, at least sometimes.

No, you're absolutely right, I just noticed the potential illegal free
here too. Perhaps namelist[n] has just happened to be NULL so far.

Thanks,
Johan