Re: [uml-devel] [PATCH 3/3] uml: os-Linux/main.c memory leak fix
On Thu, Jul 7, 2011 at 7:48 PM, Richard Weinberger wrote:
> Am Donnerstag 07 Juli 2011, 18:37:48 schrieb Vitaliy Ivanov:
>> >From 6db3c87f57e3e61d968da79f01fb21ba17fd5bc0 Mon Sep 17 00:00:00 2001
>>
>> From: Vitaliy Ivanov
>> Date: Thu, 7 Jul 2011 19:29:06 +0300
>> Subject: [PATCH 3/3] uml: os-Linux/main.c memory leak fix
>>
>> We should cleanup memory even though 'putenv' fails.
>>
>> Signed-off-by: Vitaliy Ivanov
>> ---
>> arch/um/os-Linux/main.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/um/os-Linux/main.c b/arch/um/os-Linux/main.c
>> index fb2a97a..67fe012 100644
>> --- a/arch/um/os-Linux/main.c
>> +++ b/arch/um/os-Linux/main.c
>> @@ -107,8 +107,8 @@ static void setup_env_path(void)
>> snprintf(new_path, path_len, "PATH=%s" UML_LIB_PATH, old_path);
>> if (putenv(new_path)) {
>> perror("couldn't putenv to set a new PATH");
>> - free(new_path);
>> }
>> + free(new_path);
>
> Uhhh, this will kill the PATH variable.
> putenv() does not copy the string...
That's actually depends on the version of glibc that you use. But yes,
agree, my fault. Please drop this one.
I hope I'm not in your shit-list already.:)
BTW, you said that you will share your git tree on kernel.org, is
there any news? I'm tracking what patches are accepted not to resend
them again.
Thanks,
Vitaliy
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 3/3] uml: os-Linux/main.c memory leak fix
Am Freitag 08 Juli 2011, 12:16:20 schrieb Vitaliy Ivanov:
> On Thu, Jul 7, 2011 at 7:48 PM, Richard Weinberger wrote:
> > Am Donnerstag 07 Juli 2011, 18:37:48 schrieb Vitaliy Ivanov:
> >> >From 6db3c87f57e3e61d968da79f01fb21ba17fd5bc0 Mon Sep 17 00:00:00 2001
> >>
> >> From: Vitaliy Ivanov
> >> Date: Thu, 7 Jul 2011 19:29:06 +0300
> >> Subject: [PATCH 3/3] uml: os-Linux/main.c memory leak fix
> >>
> >> We should cleanup memory even though 'putenv' fails.
> >>
> >> Signed-off-by: Vitaliy Ivanov
> >> ---
> >> arch/um/os-Linux/main.c |2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/um/os-Linux/main.c b/arch/um/os-Linux/main.c
> >> index fb2a97a..67fe012 100644
> >> --- a/arch/um/os-Linux/main.c
> >> +++ b/arch/um/os-Linux/main.c
> >> @@ -107,8 +107,8 @@ static void setup_env_path(void)
> >> snprintf(new_path, path_len, "PATH=%s" UML_LIB_PATH, old_path);
> >> if (putenv(new_path)) {
> >> perror("couldn't putenv to set a new PATH");
> >> - free(new_path);
> >> }
> >> + free(new_path);
> >
> > Uhhh, this will kill the PATH variable.
> > putenv() does not copy the string...
>
> That's actually depends on the version of glibc that you use. But yes,
> agree, my fault. Please drop this one.
> I hope I'm not in your shit-list already.:)
No problem. :)
> BTW, you said that you will share your git tree on kernel.org, is
> there any news? I'm tracking what patches are accepted not to resend
> them again.
Yeah, I'll announce it on user-mode-linux-devel@lists.sourceforge.net.
It's done when it's done. :-P
Thanks,
//richard
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 1/3] uml: drivers/net_user.c memory leak fix
On Thu, Jul 7, 2011 at 8:27 PM, Richard Weinberger wrote:
> Am Donnerstag 07 Juli 2011, 18:36:02 schrieb Vitaliy Ivanov:
>> >From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
>>
>> From: Vitaliy Ivanov
>> 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
>> ---
>> 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
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 3/3] uml: os-Linux/main.c memory leak fix
>> BTW, you said that you will share your git tree on kernel.org, is >> there any news? I'm tracking what patches are accepted not to resend >> them again. > > Yeah, I'll announce it on user-mode-linux-devel@lists.sourceforge.net. > It's done when it's done. :-P Sure. No problems. - Vitaliy -- All of the data generated in your IT infrastructure is seriously valuable. Why? It contains a definitive record of application performance, security threats, fraudulent activity, and more. Splunk takes this data and makes sense of it. IT sense. And common sense. http://p.sf.net/sfu/splunk-d2d-c2 ___ User-mode-linux-devel mailing list User-mode-linux-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 1/3] uml: drivers/net_user.c memory leak fix
Am Freitag 08 Juli 2011, 12:30:56 schrieb Vitaliy Ivanov:
> On Thu, Jul 7, 2011 at 8:27 PM, Richard Weinberger wrote:
> > Am Donnerstag 07 Juli 2011, 18:36:02 schrieb Vitaliy Ivanov:
> >> >From 9b9f36f46aa708c3245f5ded83f96421966b2edf Mon Sep 17 00:00:00 2001
> >>
> >> From: Vitaliy Ivanov
> >> 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
> >> ---
> >> 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
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 1/3] uml: drivers/net_user.c memory leak fix
> > > 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
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
Signed-off-by: Richard Weinberger
---
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
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Re: [uml-devel] [PATCH 1/3] uml: drivers/net_user.c memory leak fix
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
> 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
> Signed-off-by: Richard Weinberger
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
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
