Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid

[Aaron Conole <aconole@redhat.com>]

Ziye Yang <ziye.yang@intel.com> writes:

> From: Ziye Yang <optimistyzy@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.yang@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.

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.

>  	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;
>  		}
>  	}