* [PATCH v2] staging: greybus: loopback_test: fix device-name leak
@ 2017-02-21 19:06 Gargi Sharma
2017-02-21 19:45 ` Johan Hovold
2017-02-21 19:50 ` Johan Hovold
0 siblings, 2 replies; 6+ messages in thread
From: Gargi Sharma @ 2017-02-21 19:06 UTC (permalink / raw)
To: outreachy-kernel; +Cc: gregkh, johan, elder, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak
2017-02-21 19:06 [PATCH v2] staging: greybus: loopback_test: fix device-name leak Gargi Sharma
@ 2017-02-21 19:45 ` Johan Hovold
2017-02-21 19:50 ` Johan Hovold
1 sibling, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2017-02-21 19:45 UTC (permalink / raw)
To: Gargi Sharma; +Cc: outreachy-kernel, gregkh, johan, elder
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak
2017-02-21 19:06 [PATCH v2] staging: greybus: loopback_test: fix device-name leak Gargi Sharma
2017-02-21 19:45 ` Johan Hovold
@ 2017-02-21 19:50 ` Johan Hovold
2017-02-22 10:15 ` [Outreachy kernel] " Gargi Sharma
1 sibling, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2017-02-21 19:50 UTC (permalink / raw)
To: Gargi Sharma; +Cc: outreachy-kernel, gregkh, johan, elder
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak
2017-02-21 19:50 ` Johan Hovold
@ 2017-02-22 10:15 ` Gargi Sharma
2017-02-22 10:24 ` Julia Lawall
2017-02-22 10:37 ` Johan Hovold
0 siblings, 2 replies; 6+ messages in thread
From: Gargi Sharma @ 2017-02-22 10:15 UTC (permalink / raw)
To: Johan Hovold; +Cc: outreachy-kernel, Greg KH, elder
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak
2017-02-22 10:15 ` [Outreachy kernel] " Gargi Sharma
@ 2017-02-22 10:24 ` Julia Lawall
2017-02-22 10:37 ` Johan Hovold
1 sibling, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2017-02-22 10:24 UTC (permalink / raw)
To: Gargi Sharma; +Cc: Johan Hovold, outreachy-kernel, Greg KH, elder
[-- 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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2] staging: greybus: loopback_test: fix device-name leak
2017-02-22 10:15 ` [Outreachy kernel] " Gargi Sharma
2017-02-22 10:24 ` Julia Lawall
@ 2017-02-22 10:37 ` Johan Hovold
1 sibling, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2017-02-22 10:37 UTC (permalink / raw)
To: Gargi Sharma; +Cc: Johan Hovold, outreachy-kernel, Greg KH, elder
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-22 10:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-21 19:06 [PATCH v2] staging: greybus: loopback_test: fix device-name leak Gargi Sharma
2017-02-21 19:45 ` Johan Hovold
2017-02-21 19:50 ` Johan Hovold
2017-02-22 10:15 ` [Outreachy kernel] " Gargi Sharma
2017-02-22 10:24 ` Julia Lawall
2017-02-22 10:37 ` Johan Hovold
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.