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" <konstantin.anan...@intel.com> writes: >> -----Original Message----- >> From: Aaron Conole [mailto:acon...@redhat.com] >> Sent: Tuesday, September 11, 2018 2:47 PM >> To: Yang, Ziye <ziye.y...@intel.com> >> Cc: dev@dpdk.org; Ananyev, Konstantin >> <konstantin.anan...@intel.com>; Ziye Yang <optimist...@gmail.com> >> Subject: Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory >> leak issue of logid >> >> Ziye Yang <ziye.y...@intel.com> writes: >> >> > From: Ziye Yang <optimist...@gmail.com> >> > >> > 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 <ziye.y...@intel.com> >> > --- >> > 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; >> > } >> > }
