From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 598818E58 for ; Thu, 13 Sep 2018 15:28:08 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 892813082B64; Thu, 13 Sep 2018 13:28:07 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E68BA5D9C8; Thu, 13 Sep 2018 13:28:06 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "Yang\, Ziye" , "dev\@dpdk.org" , Ziye Yang , Thomas Monjalon References: <1536125954-46632-1-git-send-email-ziye.yang@intel.com> <1536629269-35295-1-git-send-email-ziye.yang@intel.com> <2601191342CEEE43887BDE71AB977258EA954127@irsmsx105.ger.corp.intel.com> Date: Thu, 13 Sep 2018 09:28:05 -0400 In-Reply-To: <2601191342CEEE43887BDE71AB977258EA954127@irsmsx105.ger.corp.intel.com> (Konstantin Ananyev's message of "Tue, 11 Sep 2018 14:06:36 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Thu, 13 Sep 2018 13:28:07 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Sep 2018 13:28:08 -0000 On second thought - please just fold in the patch I proposed back in July that I was told would be merged: http://mails.dpdk.org/archives/dev/2018-July/108445.html http://mails.dpdk.org/archives/dev/2018-August/109177.html It doesn't include the issues calling rte_atomic32_clear (that I have concerns about) and preserves an arbitrary length value being passed through argv/argc. :-) -Aaron "Ananyev, Konstantin" writes: >> -----Original Message----- >> From: Aaron Conole [mailto:aconole@redhat.com] >> Sent: Tuesday, September 11, 2018 2:47 PM >> To: Yang, Ziye >> Cc: dev@dpdk.org; Ananyev, Konstantin >> ; Ziye Yang >> Subject: Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory >> leak issue of logid >> >> Ziye Yang writes: >> >> > From: Ziye Yang >> > >> > This patch is used to fix the memory leak issue of logid. >> > We use the ASAN test in SPDK when intergrating DPDK and >> > find this memory leak issue. >> > >> > By the way, we also fix several missed function call of >> > rte_atomic32_clear. >> >> This part I don't understand. It should be a separate proposal. >> >> > Signed-off-by: Ziye Yang >> > --- >> > lib/librte_eal/linuxapp/eal/eal.c | 11 +++++++---- >> > 1 file changed, 7 insertions(+), 4 deletions(-) >> > >> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c >> > index e59ac65..a5129e5 100644 >> > --- a/lib/librte_eal/linuxapp/eal/eal.c >> > +++ b/lib/librte_eal/linuxapp/eal/eal.c >> > @@ -793,7 +793,8 @@ static void rte_eal_init_alert(const char *msg) >> > int i, fctret, ret; >> > pthread_t thread_id; >> > static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0); >> > - const char *logid; >> > + const char *p; >> > + static char logid[PATH_MAX]; >> >> On a linux system, PATH_MAX is 4096, but an argument may be >> MAX_ARG_STRLEN which is significantly higher. > > But we only interested here in 'basename(argv[0])'. > Surely it shouldn't be bigger than PATH_MAX unless something is > terribly wrong here. > >> >> Have you thought about an alternative where you keep the strdup and add >> an atexit() handler to do the free? Otherwise, you'll need to add code >> to check the string length as well and enforce some kind of size >> restriction. > > snprintf() below will do a safe truncation for us. > >> >> > char cpuset[RTE_CPU_AFFINITY_STR_LEN]; >> > char thread_name[RTE_MAX_THREAD_NAME_LEN]; >> > >> > @@ -810,9 +811,8 @@ static void rte_eal_init_alert(const char *msg) >> > return -1; >> > } >> > >> > - logid = strrchr(argv[0], '/'); >> > - logid = strdup(logid ? logid + 1: argv[0]); >> > - >> > + p = strrchr(argv[0], '/'); >> > + snprintf(logid, sizeof(logid), "%s", (p ? p + 1 : argv[0])); >> > thread_id = pthread_self(); >> > >> > eal_reset_internal_config(&internal_config); >> > @@ -823,6 +823,7 @@ static void rte_eal_init_alert(const char *msg) >> > if (rte_eal_cpu_init() < 0) { >> > rte_eal_init_alert("Cannot detect lcores."); >> > rte_errno = ENOTSUP; >> > + rte_atomic32_clear(&run_once); >> >> This is not recoverable. No amount of retry will allow the user to >> re-init the eal - the hardware isn't supported. Why clear the run_once >> flag? >> >> > return -1; >> > } >> > >> > @@ -851,6 +852,7 @@ static void rte_eal_init_alert(const char *msg) >> > >> > if (rte_eal_intr_init() < 0) { >> > rte_eal_init_alert("Cannot init interrupt-handling thread\n"); >> > + rte_atomic32_clear(&run_once); >> >> Arguable whether or not this is recoverable. IIRC, the eal_intr_init >> spawns a thread - if it fails to spawn the likelihood is the process >> won't be able to continue. >> >> > return -1; >> > } >> > >> > @@ -861,6 +863,7 @@ static void rte_eal_init_alert(const char *msg) >> > rte_eal_init_alert("failed to init mp channel\n"); >> > if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >> > rte_errno = EFAULT; >> > + rte_atomic32_clear(&run_once); >> >> This is also not recoverable. Why clear the run_once flag? >> >> > return -1; >> > } >> > }