From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id AFA3F4C94 for ; Tue, 11 Sep 2018 17:27:28 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A4AE940241C4; Tue, 11 Sep 2018 15:27:27 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 63D2663A7E; Tue, 11 Sep 2018 15:27:27 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "Yang\, Ziye" , "dev\@dpdk.org" , Ziye Yang 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: Tue, 11 Sep 2018 11:27:26 -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.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 11 Sep 2018 15:27:27 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 11 Sep 2018 15:27:27 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'aconole@redhat.com' RCPT:'' 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: Tue, 11 Sep 2018 15:27:29 -0000 "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. The application has full control of what it passes into EAL, but does it sanitize and scrub the arguments? We make an assumption that the argv,argc are direct from cmdline and are calls by the user at a shell. But nothing forces this to be true. >> >> 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. Anyway, yes. I completely glossed over it. >> >> > 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; >> > } >> > }