From mboxrd@z Thu Jan  1 00:00:00 1970
From: Panu Matilainen <pmatilai@redhat.com>
Subject: Re: [PATCH 1/1] lib/librte_eal: fix resource leak
Date: Fri, 22 Apr 2016 13:42:11 +0300
Message-ID: <571A0003.1040202@redhat.com>
References: <1461083251-31140-1-git-send-email-marcinx.kerlin@intel.com>
 <CALwxeUv6TV=E7H00JKTr7jwYCyswQKh=OLbBTaj2isvZwBHyuQ@mail.gmail.com>
 <5718B726.5040300@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "dev@dpdk.org" <dev@dpdk.org>, Marcin Kerlin <marcinx.kerlin@intel.com>
To: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
 David Marchand <david.marchand@6wind.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by dpdk.org (Postfix) with ESMTP id 91E9D2C4B
 for <dev@dpdk.org>; Fri, 22 Apr 2016 12:42:13 +0200 (CEST)
In-Reply-To: <5718B726.5040300@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 04/21/2016 02:19 PM, Sergio Gonzalez Monroy wrote:
> On 20/04/2016 10:15, David Marchand wrote:
>> On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin
>> <marcinx.kerlin@intel.com> wrote:
>>> Fix issue reported by Coverity.
>>>
>>> Coverity ID 13295, 13296, 13303:
>>> Resource leak: The system resource will not be reclaimed
>>> and reused, reducing the future availability of the resource.
>>> In rte_eal_hugepage_attach: Leak of memory or pointers to system
>>> resources.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>>
>>> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
>>> ---
>>>   lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> index 5b9132c..6320aa0 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> @@ -1475,13 +1475,17 @@ rte_eal_hugepage_attach(void)
>>>                                          "and retry running both
>>> primary "
>>>                                          "and secondary processes\n");
>>>                          }
>>> +
>>> +                       if (base_addr != MAP_FAILED)
>>> +                               munmap((void *)(uintptr_t)base_addr,
>>> mcfg->memseg[s].len);
>>> +
>> What is the point of this casting ?
>> Idem for the rest of the patch.
>
> I don't see the point either.
> Marcin?
>
>>
>> I can't see cleanup for previously mapped segments when mapping one
>> fails.
>> Do we want this cleanup as well ?
>
> Good point.
>
> We are not really leaking resources because we panic if we fail to
> initialize eal memory.

FWIW, the panic-attack mode is something I'd like to see eliminated 
eventually and hopefully will be submitting patches for sooner or later. 
Aborting from library code is rather antisocial behavior, even if its on 
just initialization code that usually runs fairly early in process lifetime.

>
> Now, if we are going to do the cleanup, I think that as David points out
> we should be
> cleaning up all previous mappings too.

+1

Even if the current code just panics, it doesn't mean it always will.

	- Panu -

>
> Sergio
>> CC Sergio.
>>
>>
>