From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753315AbaHTWGw (ORCPT <rfc822;w@1wt.eu>);
	Wed, 20 Aug 2014 18:06:52 -0400
Received: from ozlabs.org ([103.22.144.67]:51002 "EHLO ozlabs.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753210AbaHTWGv (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 20 Aug 2014 18:06:51 -0400
From: Rusty Russell <rusty@rustcorp.com.au>
To: Arjun Sreedharan <arjun024@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Jingoo Han <jg1.han@samsung.com>, linux-kernel@vger.kernel.org,
        David Woodhouse <David.Woodhouse@intel.com>
Subject: Re: [PATCH] params: fix potential memory leak in add_sysfs_param()
In-Reply-To: <CAJFMrCG1OXrdRoUNqtZYdBqE_0dj+a3yAfFhuyFvTte7Ozbo0w@mail.gmail.com>
References: <1408564816-1778-1-git-send-email-arjun024@gmail.com> <87mwayj39c.fsf@rustcorp.com.au> <CAJFMrCG1OXrdRoUNqtZYdBqE_0dj+a3yAfFhuyFvTte7Ozbo0w@mail.gmail.com>
User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-pc-linux-gnu)
Date: Thu, 21 Aug 2014 07:35:33 +0930
Message-ID: <87ha16izpu.fsf@rustcorp.com.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.