* [PATCH 1/3] uml: drivers/net_user.c memory leak fix
@ 2011-07-07 16:36 Vitaliy Ivanov
2011-07-07 17:27 ` Richard Weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Vitaliy Ivanov @ 2011-07-07 16:36 UTC (permalink / raw)
To: Jeff Dike, Richard Weinberger; +Cc: user-mode-linux-devel, LKML, vitalivanov
>From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
From: Vitaliy Ivanov <vitalivanov@gmail.com>
Date: Thu, 7 Jul 2011 19:23:13 +0300
Subject: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
Perform memory cleanup on exit.
On receiving invalid 'pid' we still should clean 'output' variable.
Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
---
arch/um/drivers/net_user.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
index 9415dd9..989b653 100644
--- a/arch/um/drivers/net_user.c
+++ b/arch/um/drivers/net_user.c
@@ -228,10 +228,11 @@ static void change(char *dev, char *what, unsigned char *addr,
"buffer\n");
pid = change_tramp(argv, output, output_len);
- if (pid < 0) return;
if (output != NULL) {
- printk("%s", output);
+ if (pid >= 0) {
+ printk("%s", output);
+ }
kfree(output);
}
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
2011-07-07 16:36 [PATCH 1/3] uml: drivers/net_user.c memory leak fix Vitaliy Ivanov
@ 2011-07-07 17:27 ` Richard Weinberger
2011-07-08 10:30 ` Vitaliy Ivanov
0 siblings, 1 reply; 6+ messages in thread
From: Richard Weinberger @ 2011-07-07 17:27 UTC (permalink / raw)
To: Vitaliy Ivanov; +Cc: Jeff Dike, user-mode-linux-devel, LKML
Am Donnerstag 07 Juli 2011, 18:36:02 schrieb Vitaliy Ivanov:
> >From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
>
> From: Vitaliy Ivanov <vitalivanov@gmail.com>
> Date: Thu, 7 Jul 2011 19:23:13 +0300
> Subject: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
>
> Perform memory cleanup on exit.
> On receiving invalid 'pid' we still should clean 'output' variable.
>
> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
> ---
> arch/um/drivers/net_user.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
> index 9415dd9..989b653 100644
> --- a/arch/um/drivers/net_user.c
> +++ b/arch/um/drivers/net_user.c
> @@ -228,10 +228,11 @@ static void change(char *dev, char *what, unsigned
> char *addr, "buffer\n");
>
> pid = change_tramp(argv, output, output_len);
> - if (pid < 0) return;
>
> if (output != NULL) {
> - printk("%s", output);
> + if (pid >= 0) {
> + printk("%s", output);
> + }
> kfree(output);
> }
> }
This control logic is a bit strange.
When change_tramp() fails we should not printk() the output variable.
if (pid < 0){
free(output);
return;
}
Would be much cleaner.
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
2011-07-07 17:27 ` Richard Weinberger
@ 2011-07-08 10:30 ` Vitaliy Ivanov
2011-07-08 10:44 ` Richard Weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Vitaliy Ivanov @ 2011-07-08 10:30 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Jeff Dike, user-mode-linux-devel, LKML
On Thu, Jul 7, 2011 at 8:27 PM, Richard Weinberger <richard@nod.at> wrote:
> Am Donnerstag 07 Juli 2011, 18:36:02 schrieb Vitaliy Ivanov:
>> >From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
>>
>> From: Vitaliy Ivanov <vitalivanov@gmail.com>
>> Date: Thu, 7 Jul 2011 19:23:13 +0300
>> Subject: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
>>
>> Perform memory cleanup on exit.
>> On receiving invalid 'pid' we still should clean 'output' variable.
>>
>> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
>> ---
>> arch/um/drivers/net_user.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
>> index 9415dd9..989b653 100644
>> --- a/arch/um/drivers/net_user.c
>> +++ b/arch/um/drivers/net_user.c
>> @@ -228,10 +228,11 @@ static void change(char *dev, char *what, unsigned
>> char *addr, "buffer\n");
>>
>> pid = change_tramp(argv, output, output_len);
>> - if (pid < 0) return;
>>
>> if (output != NULL) {
>> - printk("%s", output);
>> + if (pid >= 0) {
>> + printk("%s", output);
>> + }
>> kfree(output);
>> }
>> }
>
> This control logic is a bit strange.
> When change_tramp() fails we should not printk() the output variable.
>
> if (pid < 0){
> free(output);
> return;
> }
>
> Would be much cleaner.
I just didn't want to clone this free-return stuff. So, what you
proposing is like this:
------------
...
output = uml_kmalloc(output_len, UM_GFP_KERNEL);
if (output == NULL)
printk(UM_KERN_ERR "change : failed to allocate output "
"buffer\n");
pid = change_tramp(argv, output, output_len);
if (pid < 0) {
free(output); <---------- I'm not sure
but 'output' can be NULL here.
return;
}
if (output != NULL) {
printk("%s", output);
kfree(output);
}
}
------------
I was trying to print 'output' only in case change_tramp is
successful. That's the old logic. And at the same time perform free
only in case output is not NULL.
Vitaliy
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
2011-07-08 10:30 ` Vitaliy Ivanov
@ 2011-07-08 10:44 ` Richard Weinberger
2011-07-08 13:00 ` Vitaliy Ivanov
0 siblings, 1 reply; 6+ messages in thread
From: Richard Weinberger @ 2011-07-08 10:44 UTC (permalink / raw)
To: Vitaliy Ivanov; +Cc: Jeff Dike, user-mode-linux-devel, LKML
Am Freitag 08 Juli 2011, 12:30:56 schrieb Vitaliy Ivanov:
> On Thu, Jul 7, 2011 at 8:27 PM, Richard Weinberger <richard@nod.at> wrote:
> > Am Donnerstag 07 Juli 2011, 18:36:02 schrieb Vitaliy Ivanov:
> >> >From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
> >>
> >> From: Vitaliy Ivanov <vitalivanov@gmail.com>
> >> Date: Thu, 7 Jul 2011 19:23:13 +0300
> >> Subject: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
> >>
> >> Perform memory cleanup on exit.
> >> On receiving invalid 'pid' we still should clean 'output' variable.
> >>
> >> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
> >> ---
> >> arch/um/drivers/net_user.c | 5 +++--
> >> 1 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
> >> index 9415dd9..989b653 100644
> >> --- a/arch/um/drivers/net_user.c
> >> +++ b/arch/um/drivers/net_user.c
> >> @@ -228,10 +228,11 @@ static void change(char *dev, char *what, unsigned
> >> char *addr, "buffer\n");
> >>
> >> pid = change_tramp(argv, output, output_len);
> >> - if (pid < 0) return;
> >>
> >> if (output != NULL) {
> >> - printk("%s", output);
> >> + if (pid >= 0) {
> >> + printk("%s", output);
> >> + }
> >> kfree(output);
> >> }
> >> }
> >
> > This control logic is a bit strange.
> > When change_tramp() fails we should not printk() the output variable.
> >
> > if (pid < 0){
> > free(output);
> > return;
> > }
> >
> > Would be much cleaner.
>
> I just didn't want to clone this free-return stuff. So, what you
> proposing is like this:
> ------------
> ...
> output = uml_kmalloc(output_len, UM_GFP_KERNEL);
> if (output == NULL)
> printk(UM_KERN_ERR "change : failed to allocate output "
> "buffer\n");
>
> pid = change_tramp(argv, output, output_len);
> if (pid < 0) {
> free(output); <---------- I'm not sure
> but 'output' can be NULL here.
> return;
> }
>
> if (output != NULL) {
> printk("%s", output);
> kfree(output);
> }
> }
> ------------
>
> I was trying to print 'output' only in case change_tramp is
> successful. That's the old logic. And at the same time perform free
> only in case output is not NULL.
Why?
Freeing a NULL pointer is perfectly fine.
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
2011-07-08 10:44 ` Richard Weinberger
@ 2011-07-08 13:00 ` Vitaliy Ivanov
2011-07-08 15:42 ` Richard Weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Vitaliy Ivanov @ 2011-07-08 13:00 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Jeff Dike, user-mode-linux-devel, LKML
> > > This control logic is a bit strange.
> > > When change_tramp() fails we should not printk() the output variable.
> > >
> > > if (pid < 0){
> > > free(output);
> > > return;
> > > }
> > >
> > > Would be much cleaner.
> >
> > I just didn't want to clone this free-return stuff. So, what you
> > proposing is like this:
> > ------------
> > ...
> > output = uml_kmalloc(output_len, UM_GFP_KERNEL);
> > if (output == NULL)
> > printk(UM_KERN_ERR "change : failed to allocate output "
> > "buffer\n");
> >
> > pid = change_tramp(argv, output, output_len);
> > if (pid < 0) {
> > free(output); <---------- I'm not sure but 'output' can be NULL here.
> > return;
> > }
> >
> > if (output != NULL) {
> > printk("%s", output);
> > kfree(output);
> > }
> > }
> > ------------
> >
> > I was trying to print 'output' only in case change_tramp is
> > successful. That's the old logic. And at the same time perform free
> > only in case output is not NULL.
>
> Why?
> Freeing a NULL pointer is perfectly fine.
I should agree that something that you propose has better readability.
So, here is updated patch.
Thanks
---
>From b0c5ca0336cc94b2fda251e0da7918394e59c5cd Mon Sep 17 00:00:00 2001
From: Vitaliy Ivanov <vitalivanov@gmail.com>
Date: Fri, 8 Jul 2011 14:54:29 +0300
Subject: [PATCH] uml: drivers/net_user.c memory leak fix
Perform memory cleanup on exit.
On receiving invalid 'pid' we still should clean 'output' variable.
Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
arch/um/drivers/net_user.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
index 9415dd9..5201188 100644
--- a/arch/um/drivers/net_user.c
+++ b/arch/um/drivers/net_user.c
@@ -228,7 +228,10 @@ static void change(char *dev, char *what, unsigned char *addr,
"buffer\n");
pid = change_tramp(argv, output, output_len);
- if (pid < 0) return;
+ if (pid < 0) {
+ kfree(output);
+ return;
+ }
if (output != NULL) {
printk("%s", output);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] uml: drivers/net_user.c memory leak fix
2011-07-08 13:00 ` Vitaliy Ivanov
@ 2011-07-08 15:42 ` Richard Weinberger
0 siblings, 0 replies; 6+ messages in thread
From: Richard Weinberger @ 2011-07-08 15:42 UTC (permalink / raw)
To: Vitaliy Ivanov; +Cc: Jeff Dike, user-mode-linux-devel, LKML
Am Freitag 08 Juli 2011, 15:00:44 schrieb Vitaliy Ivanov:
> > > > This control logic is a bit strange.
> > > > When change_tramp() fails we should not printk() the output variable.
> > > >
> > > > if (pid < 0){
> > > >
> > > > free(output);
> > > > return;
> > > >
> > > > }
> > > >
> > > > Would be much cleaner.
> > >
> > > I just didn't want to clone this free-return stuff. So, what you
> > > proposing is like this:
> > > ------------
> > > ...
> > >
> > > output = uml_kmalloc(output_len, UM_GFP_KERNEL);
> > > if (output == NULL)
> > >
> > > printk(UM_KERN_ERR "change : failed to allocate output
> > > "
> > >
> > > "buffer\n");
> > >
> > > pid = change_tramp(argv, output, output_len);
> > > if (pid < 0) {
> > >
> > > free(output); <---------- I'm not sure
> > > but 'output' can be NULL here. return;
> > >
> > > }
> > >
> > > if (output != NULL) {
> > >
> > > printk("%s", output);
> > > kfree(output);
> > >
> > > }
> > >
> > > }
> > > ------------
> > >
> > > I was trying to print 'output' only in case change_tramp is
> > > successful. That's the old logic. And at the same time perform free
> > > only in case output is not NULL.
> >
> > Why?
> > Freeing a NULL pointer is perfectly fine.
>
> I should agree that something that you propose has better readability.
>
> So, here is updated patch.
>
> Thanks
> ---
>
> >From b0c5ca0336cc94b2fda251e0da7918394e59c5cd Mon Sep 17 00:00:00 2001
>
> From: Vitaliy Ivanov <vitalivanov@gmail.com>
> Date: Fri, 8 Jul 2011 14:54:29 +0300
> Subject: [PATCH] uml: drivers/net_user.c memory leak fix
>
> Perform memory cleanup on exit.
> On receiving invalid 'pid' we still should clean 'output' variable.
>
> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
Why are you adding my Signed-off-by?!
That's my job...
> ---
> arch/um/drivers/net_user.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/um/drivers/net_user.c b/arch/um/drivers/net_user.c
> index 9415dd9..5201188 100644
> --- a/arch/um/drivers/net_user.c
> +++ b/arch/um/drivers/net_user.c
> @@ -228,7 +228,10 @@ static void change(char *dev, char *what, unsigned
> char *addr, "buffer\n");
>
> pid = change_tramp(argv, output, output_len);
> - if (pid < 0) return;
> + if (pid < 0) {
> + kfree(output);
> + return;
> + }
>
> if (output != NULL) {
> printk("%s", output);
Anyway, Applied!
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-08 15:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-07 16:36 [PATCH 1/3] uml: drivers/net_user.c memory leak fix Vitaliy Ivanov
2011-07-07 17:27 ` Richard Weinberger
2011-07-08 10:30 ` Vitaliy Ivanov
2011-07-08 10:44 ` Richard Weinberger
2011-07-08 13:00 ` Vitaliy Ivanov
2011-07-08 15:42 ` Richard Weinberger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox