Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH] params: fix potential memory leak in add_sysfs_param() | Date | Thu, 21 Aug 2014 07:35:33 +0930 |
| |
Arjun Sreedharan <arjun024@gmail.com> writes: > On 21 August 2014 02:19, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Arjun Sreedharan <arjun024@gmail.com> writes: >>> Do not leak memory when attrs is non NULL and >>> krealloc() fails. Without temporary variable, >>> reference to it is lost. >>> >>> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com> >> >> ... >> >>> } >>> - /* Despite looking like the typical realloc() bug, this is safe. >>> - * We *want* the old 'attrs' to be freed either way, and we'll store >>> - * the new one in the success case. */ >>> - attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL); >>> - if (!attrs) { >>> + >>> + new_attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL); >>> + if (!new_attrs) { >> >> I think that comment you deleted is pretty clear. Is it wrong? >> >> Cheers, >> Rusty. > > I believe it's wrong. I do not understand how `this is safe` from memory leak. > If krealloc() fails, there is nothing in place to free memory held by @attrs
Above this: if (!mk->mp) { num = 0; attrs = NULL; } else { num = mk->mp->num; attrs = mk->mp->grp.attrs; }
So, attrs is just a temporary: either NULL (doesn't need freeing), or is the old mk->mp->grp.attrs ptr.
But David was the one who placed this comment; I'll let him figure out whether it's bogus or not :)
Cheers, Rusty.
| |