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

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

[Gargi Sharma]

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;
-- 
2.7.4

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

[Johan Hovold]

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.

Reviewed-by: Johan Hovold <johan@kernel.org>

Thanks,
Johan

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

[Johan Hovold]

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.

No need to resend, unless you prefer. You can add my Reviewed-by tag if
so.

Thanks,
Johan

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

[Gargi Sharma]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

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 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.

Thanks,
Gargi
>
> Thanks,
> Johan
>
> --

[-- Attachment #2: Type: text/html, Size: 2641 bytes --]

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 3133 bytes --]



On Wed, 22 Feb 2017, 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 change eliminates the illegal free.

scandir is returning an array of n elements.  In such an array, the
elements have numbers 0 .. n-1.  namelist[n] doesn't exist.  If one is
very lucky, there is a NULL after the end of the array, and then the free
does nothing.  But doing nothing is not the goal here.  The goal is to
free the elements 0 .. n-1.  They have all been allocated, according to
how scandir works, so it will be fine to free them.

julia

>
> > 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.
>
> Thanks,
> Gargi
> >
> > Thanks,
> > Johan
> >
> > --
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CAOCi2DHkHpakjYezcnCXF35
> ViB5%2BLW174r_%3DAYMCt22OYg3X5g%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

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

[Johan Hovold]

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