Messages in this thread Patch in this message |  | | | Date | Sat, 27 Jun 2009 02:14:50 +0300 | | From | Sergey Senozhatsky <> | | Subject | Re: [PATCH] kobject_set_name_vargs memory leak |
| |
On (06/26/09 16:00), Eric W. Biederman wrote: > >> > Fix memory leak when kobject_set_name_vargs returns -ENOMEM. > >> > > >> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by> > >> > --- > >> > diff --git a/lib/kobject.c b/lib/kobject.c > >> > index b512b74..922cd8c 100644 > >> > --- a/lib/kobject.c > >> > +++ b/lib/kobject.c > >> > @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, > >> > return 0; > >> > > >> > kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); > >> > - if (!kobj->name) > >> > + if (!kobj->name) { > >> > + kfree(old_name); > >> > return -ENOMEM; > >> > + } > >> > >> We've been through this before (search lkml archives). If kvasprintf > >> fails, then we don't want to free old_name, as the caller might want to > >> do something with it. > >> > > Hello Greg, > > > > int kobject_set_name_vargs.... { > > const char *old_name = kobj->name; > > > > old_name is local variable. > > > > In the following lines we overwrite kobject->name. > > > > kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); > > if (!kobj->name) > > return -ENOMEM; > > > > It's not clear to me how we can do anything (including kfree) with old_name after 'return -ENOMEM'. > > My feel is that if we fail we should restore kobject->name to old_name. > > That should also prevent the leak without getting us into trouble elsewhere. > > Eric > Or work with 'new_name' and overwrite kobject->name only 'if(new_name)'. I thought about restoring. ( Blue or Red Pill? :) )
diff --git a/lib/kobject.c b/lib/kobject.c index b512b74..d6b1502 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, return 0; kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); - if (!kobj->name) + if (!kobj->name) { + kobj->name = old_name; return -ENOMEM; + } /* ewww... some of these buggers have '/' in the name ... */ while ((s = strchr(kobj->name, '/')))
Sergey
|  |