* [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs
@ 2010-03-13 12:43 Philippe De Muyter
2010-03-13 12:53 ` Robert P. J. Day
0 siblings, 1 reply; 4+ messages in thread
From: Philippe De Muyter @ 2010-03-13 12:43 UTC (permalink / raw)
To: gregkh, linux-kernel
Hello Greg,
This is a possible memory leak that I discovered only by accidental code
reading.
--
If kvasprintf fails in kobject_set_name_vargs, the memory used by
the original kobj->name is leaked. Fix that. I also avoid useless
memory accesses to kobj->name by using the local variables old_name
and new_name instead.
Signed-off-by: Philippe De Muyter <phdm@macqel.be>
diff -r 373fdd3df333 linux-2.6.x/lib/kobject.c
--- a/linux-2.6.x/lib/kobject.c Wed Aug 19 23:26:44 2009 +0200
+++ b/linux-2.6.x/lib/kobject.c Sat Mar 13 13:35:43 2010 +0100
@@ -216,20 +216,22 @@ int kobject_set_name_vargs(struct kobjec
va_list vargs)
{
const char *old_name = kobj->name;
+ char *new_name;
char *s;
- if (kobj->name && !fmt)
+ if (old_name && !fmt)
return 0;
- kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
- if (!kobj->name)
+ new_name = kvasprintf(GFP_KERNEL, fmt, vargs);
+ if (!new_name)
return -ENOMEM;
/* ewww... some of these buggers have '/' in the name ... */
- while ((s = strchr(kobj->name, '/')))
+ while ((s = strchr(new_name, '/')))
s[0] = '!';
kfree(old_name);
+ kobj->name = new_name;
return 0;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs 2010-03-13 12:43 [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs Philippe De Muyter @ 2010-03-13 12:53 ` Robert P. J. Day 2010-03-13 15:15 ` Eric Dumazet 0 siblings, 1 reply; 4+ messages in thread From: Robert P. J. Day @ 2010-03-13 12:53 UTC (permalink / raw) To: Philippe De Muyter; +Cc: gregkh, linux-kernel On Sat, 13 Mar 2010, Philippe De Muyter wrote: > Hello Greg, > > This is a possible memory leak that I discovered only by accidental code > reading. > > -- > > If kvasprintf fails in kobject_set_name_vargs, the memory used by > the original kobj->name is leaked. Fix that. I also avoid useless > memory accesses to kobj->name by using the local variables old_name > and new_name instead. > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > > diff -r 373fdd3df333 linux-2.6.x/lib/kobject.c > --- a/linux-2.6.x/lib/kobject.c Wed Aug 19 23:26:44 2009 +0200 > +++ b/linux-2.6.x/lib/kobject.c Sat Mar 13 13:35:43 2010 +0100 > @@ -216,20 +216,22 @@ int kobject_set_name_vargs(struct kobjec > va_list vargs) > { > const char *old_name = kobj->name; > + char *new_name; > char *s; > > - if (kobj->name && !fmt) > + if (old_name && !fmt) > return 0; > > - kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); > - if (!kobj->name) > + new_name = kvasprintf(GFP_KERNEL, fmt, vargs); > + if (!new_name) > return -ENOMEM; > > /* ewww... some of these buggers have '/' in the name ... */ > - while ((s = strchr(kobj->name, '/'))) > + while ((s = strchr(new_name, '/'))) > s[0] = '!'; > > kfree(old_name); > + kobj->name = new_name; > return 0; > } the routine kobject_set_name_vargs() is described in Documentation/kobject.txt as "legacy cruft" to be removed at some point, so it's not clear there's any value in "fixing" it. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Kernel Pedantry. Web page: http://crashcourse.ca Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs 2010-03-13 12:53 ` Robert P. J. Day @ 2010-03-13 15:15 ` Eric Dumazet 2010-03-13 15:27 ` Robert P. J. Day 0 siblings, 1 reply; 4+ messages in thread From: Eric Dumazet @ 2010-03-13 15:15 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Philippe De Muyter, gregkh, linux-kernel Le samedi 13 mars 2010 à 07:53 -0500, Robert P. J. Day a écrit : > On Sat, 13 Mar 2010, Philippe De Muyter wrote: > > > Hello Greg, > > > > This is a possible memory leak that I discovered only by accidental code > > reading. > > > > -- > > > > If kvasprintf fails in kobject_set_name_vargs, the memory used by > > the original kobj->name is leaked. Fix that. I also avoid useless > > memory accesses to kobj->name by using the local variables old_name > > and new_name instead. > > > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > > > > diff -r 373fdd3df333 linux-2.6.x/lib/kobject.c > > --- a/linux-2.6.x/lib/kobject.c Wed Aug 19 23:26:44 2009 +0200 > > +++ b/linux-2.6.x/lib/kobject.c Sat Mar 13 13:35:43 2010 +0100 > > @@ -216,20 +216,22 @@ int kobject_set_name_vargs(struct kobjec > > va_list vargs) > > { > > const char *old_name = kobj->name; > > + char *new_name; > > char *s; > > > > - if (kobj->name && !fmt) > > + if (old_name && !fmt) > > return 0; > > > > - kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); > > - if (!kobj->name) > > + new_name = kvasprintf(GFP_KERNEL, fmt, vargs); > > + if (!new_name) > > return -ENOMEM; > > > > /* ewww... some of these buggers have '/' in the name ... */ > > - while ((s = strchr(kobj->name, '/'))) > > + while ((s = strchr(new_name, '/'))) > > s[0] = '!'; > > > > kfree(old_name); > > + kobj->name = new_name; > > return 0; > > } > > the routine kobject_set_name_vargs() is described in > Documentation/kobject.txt as "legacy cruft" to be removed at some > point, so it's not clear there's any value in "fixing" it. > Given I submitted a similar patch two days before, I guess a fix would be welcome or else we might see one or two attempts per week from various people. http://lkml.org/lkml/2010/3/11/438 Legacy or not, this code looks wrong. I caught it while looking for kmemleaks reports on my dev machine, that were triggered by CONFIG_NO_BOOTMEM use. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs 2010-03-13 15:15 ` Eric Dumazet @ 2010-03-13 15:27 ` Robert P. J. Day 0 siblings, 0 replies; 4+ messages in thread From: Robert P. J. Day @ 2010-03-13 15:27 UTC (permalink / raw) To: Eric Dumazet; +Cc: Philippe De Muyter, gregkh, linux-kernel [-- Attachment #1: Type: TEXT/PLAIN, Size: 2699 bytes --] On Sat, 13 Mar 2010, Eric Dumazet wrote: > Le samedi 13 mars 2010 à 07:53 -0500, Robert P. J. Day a écrit : > > On Sat, 13 Mar 2010, Philippe De Muyter wrote: > > > > > Hello Greg, > > > > > > This is a possible memory leak that I discovered only by accidental code > > > reading. > > > > > > -- > > > > > > If kvasprintf fails in kobject_set_name_vargs, the memory used by > > > the original kobj->name is leaked. Fix that. I also avoid useless > > > memory accesses to kobj->name by using the local variables old_name > > > and new_name instead. > > > > > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > > > > > > diff -r 373fdd3df333 linux-2.6.x/lib/kobject.c > > > --- a/linux-2.6.x/lib/kobject.c Wed Aug 19 23:26:44 2009 +0200 > > > +++ b/linux-2.6.x/lib/kobject.c Sat Mar 13 13:35:43 2010 +0100 > > > @@ -216,20 +216,22 @@ int kobject_set_name_vargs(struct kobjec > > > va_list vargs) > > > { > > > const char *old_name = kobj->name; > > > + char *new_name; > > > char *s; > > > > > > - if (kobj->name && !fmt) > > > + if (old_name && !fmt) > > > return 0; > > > > > > - kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); > > > - if (!kobj->name) > > > + new_name = kvasprintf(GFP_KERNEL, fmt, vargs); > > > + if (!new_name) > > > return -ENOMEM; > > > > > > /* ewww... some of these buggers have '/' in the name ... */ > > > - while ((s = strchr(kobj->name, '/'))) > > > + while ((s = strchr(new_name, '/'))) > > > s[0] = '!'; > > > > > > kfree(old_name); > > > + kobj->name = new_name; > > > return 0; > > > } > > > > the routine kobject_set_name_vargs() is described in > > Documentation/kobject.txt as "legacy cruft" to be removed at some > > point, so it's not clear there's any value in "fixing" it. > > > > Given I submitted a similar patch two days before, I guess a fix > would be welcome or else we might see one or two attempts per week > from various people. > > http://lkml.org/lkml/2010/3/11/438 > > Legacy or not, this code looks wrong. I caught it while looking for > kmemleaks reports on my dev machine, that were triggered by > CONFIG_NO_BOOTMEM use. fair enough, i was just going off of what i read in Doc/kobject.txt. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Kernel Pedantry. Web page: http://crashcourse.ca Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-13 15:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-13 12:43 [PATCH kobjects] Fix a rare memory leak in kobject_set_name_vargs Philippe De Muyter 2010-03-13 12:53 ` Robert P. J. Day 2010-03-13 15:15 ` Eric Dumazet 2010-03-13 15:27 ` Robert P. J. Day
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.