diff mbox series

[v5] linuxapp, eal: Fix the memory leak issue of logid

Message ID 1536125954-46632-1-git-send-email-ziye.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v5] linuxapp, eal: Fix the memory leak issue of logid |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ziye Yang Sept. 5, 2018, 5:39 a.m. UTC 
  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.

Signed-off-by: Ziye Yang <ziye.yang@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal.c     | 21 ++++++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++++++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

Comments

Ananyev, Konstantin Sept. 10, 2018, 11:54 a.m. UTC | #1 
Hi 

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ziye Yang
> Sent: Wednesday, September 5, 2018 6:39 AM
> To: dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
> 
> 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.
> 
> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c     | 21 ++++++++++++++++++++-
>  lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++++++++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index e59ac65..e150200 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -793,7 +793,7 @@ 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;
> +	char *logid;
>  	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
>  	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> 
> @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
> 
>  	logid = strrchr(argv[0], '/');
>  	logid = strdup(logid ? logid + 1: argv[0]);
> +	if (!logid) {
> +		rte_eal_init_alert("Cannot allocate memory for logid\n");
> +		rte_errno = ENOMEM;
> +		rte_atomic32_clear(&run_once);
> +		return -1;
> +	}
> 
>  	thread_id = pthread_self();
> 
> @@ -823,6 +829,8 @@ 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);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Invalid 'command line' arguments.");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot init plugins\n");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (eal_option_device_parse()) {
>  		rte_errno = ENODEV;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -851,6 +862,8 @@ 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);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -861,6 +874,8 @@ 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);
> +			free(logid);
>  			return -1;
>  		}
>  	}
> @@ -869,6 +884,7 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot scan the buses for devices\n");
>  		rte_errno = ENODEV;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -893,6 +909,7 @@ static void rte_eal_init_alert(const char *msg)
>  			rte_eal_init_alert("Cannot get hugepage information.");
>  			rte_errno = EACCES;
>  			rte_atomic32_clear(&run_once);
> +			free(logid);
>  			return -1;
>  		}
>  	}
> @@ -919,8 +936,10 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot init logging.");
>  		rte_errno = ENOMEM;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> +	free(logid);
> 
>  #ifdef VFIO_PRESENT
>  	if (rte_eal_vfio_setup() < 0) {
> diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
> index 9d02ddd..076b1b9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_log.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_log.c
> @@ -19,6 +19,9 @@
> 
>  #include "eal_private.h"
> 
> +#define DEFAULT_LOG_ID_LEN 128
> +char g_logid[DEFAULT_LOG_ID_LEN];
> +
>  /*
>   * default log function
>   */
> @@ -49,12 +52,18 @@
>  rte_eal_log_init(const char *id, int facility)
>  {
>  	FILE *log_stream;
> +	int str_len;
> 
>  	log_stream = fopencookie(NULL, "w+", console_log_func);
>  	if (log_stream == NULL)
>  		return -1;
> 
> -	openlog(id, LOG_NDELAY | LOG_PID, facility);
> +	str_len = strlen(id);
> +	if (str_len >= DEFAULT_LOG_ID_LEN)
> +		return -1;
> +
> +	strncpy(g_logid, id, str_len);
> +	openlog(g_logid, LOG_NDELAY | LOG_PID, facility);


Could you explain what is the point here to create a global here and pass pointer to it to openlog()?
Konstantin

> 
>  	eal_log_set_default(log_stream);
> 
> --
> 1.9.3
Ziye Yang Sept. 10, 2018, 11:58 a.m. UTC | #2 
Hi Konstantin,

The global logid is used to prevent the memory leak. If we do not do this, but directly free(logid), this pointer will still be used by openlog, which means that we cannot free the logid anymore. And by adding the global variable, it will not only solve the memory leak but also makes the openlog function still work.


Best Regards
Ziye Yang 

-----Original Message-----
From: Ananyev, Konstantin 
Sent: Monday, September 10, 2018 7:55 PM
To: Yang, Ziye <ziye.yang@intel.com>; dev@dpdk.org
Cc: Ziye Yang <optimistyzy@gmail.com>
Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid

Hi 

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ziye Yang
> Sent: Wednesday, September 5, 2018 6:39 AM
> To: dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak 
> issue of logid
> 
> 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.
> 
> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c     | 21 ++++++++++++++++++++-
>  lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++++++++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
> b/lib/librte_eal/linuxapp/eal/eal.c
> index e59ac65..e150200 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -793,7 +793,7 @@ 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;
> +	char *logid;
>  	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
>  	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> 
> @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
> 
>  	logid = strrchr(argv[0], '/');
>  	logid = strdup(logid ? logid + 1: argv[0]);
> +	if (!logid) {
> +		rte_eal_init_alert("Cannot allocate memory for logid\n");
> +		rte_errno = ENOMEM;
> +		rte_atomic32_clear(&run_once);
> +		return -1;
> +	}
> 
>  	thread_id = pthread_self();
> 
> @@ -823,6 +829,8 @@ 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);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Invalid 'command line' arguments.");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot init plugins\n");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (eal_option_device_parse()) {
>  		rte_errno = ENODEV;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -851,6 +862,8 @@ 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);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -861,6 +874,8 @@ 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);
> +			free(logid);
>  			return -1;
>  		}
>  	}
> @@ -869,6 +884,7 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot scan the buses for devices\n");
>  		rte_errno = ENODEV;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -893,6 +909,7 @@ static void rte_eal_init_alert(const char *msg)
>  			rte_eal_init_alert("Cannot get hugepage information.");
>  			rte_errno = EACCES;
>  			rte_atomic32_clear(&run_once);
> +			free(logid);
>  			return -1;
>  		}
>  	}
> @@ -919,8 +936,10 @@ static void rte_eal_init_alert(const char *msg)
>  		rte_eal_init_alert("Cannot init logging.");
>  		rte_errno = ENOMEM;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> +	free(logid);
> 
>  #ifdef VFIO_PRESENT
>  	if (rte_eal_vfio_setup() < 0) {
> diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c 
> b/lib/librte_eal/linuxapp/eal/eal_log.c
> index 9d02ddd..076b1b9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_log.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_log.c
> @@ -19,6 +19,9 @@
> 
>  #include "eal_private.h"
> 
> +#define DEFAULT_LOG_ID_LEN 128
> +char g_logid[DEFAULT_LOG_ID_LEN];
> +
>  /*
>   * default log function
>   */
> @@ -49,12 +52,18 @@
>  rte_eal_log_init(const char *id, int facility)  {
>  	FILE *log_stream;
> +	int str_len;
> 
>  	log_stream = fopencookie(NULL, "w+", console_log_func);
>  	if (log_stream == NULL)
>  		return -1;
> 
> -	openlog(id, LOG_NDELAY | LOG_PID, facility);
> +	str_len = strlen(id);
> +	if (str_len >= DEFAULT_LOG_ID_LEN)
> +		return -1;
> +
> +	strncpy(g_logid, id, str_len);
> +	openlog(g_logid, LOG_NDELAY | LOG_PID, facility);


Could you explain what is the point here to create a global here and pass pointer to it to openlog()?
Konstantin

> 
>  	eal_log_set_default(log_stream);
> 
> --
> 1.9.3
Ananyev, Konstantin Sept. 10, 2018, 12:19 p.m. UTC | #3 
> -----Original Message-----
> From: Yang, Ziye
> Sent: Monday, September 10, 2018 12:58 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
> 
> Hi Konstantin,
> 
> The global logid is used to prevent the memory leak. If we do not do this, but directly free(logid), this pointer will still be used by
> openlog, which means that we cannot free the logid anymore. And by adding the global variable, it will not only solve the memory
> leak but also makes the openlog function still work.

But then I suppose you can avoid any dynamic memory allocation at all with something like that:
rte_eal_init(int argc, char **argv)
{
   ...
   static char logid[PATH_MAX];
   ...
   const char *p= strrchr(argv[0], '/');
   snprintf(logid, sizeof(logid), "%s", p);
   ...
   rte_eal_log_init(logid, ...);

?
Konstantin

> 
> 
> Best Regards
> Ziye Yang
> 
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, September 10, 2018 7:55 PM
> To: Yang, Ziye <ziye.yang@intel.com>; dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
> 
> Hi
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ziye Yang
> > Sent: Wednesday, September 5, 2018 6:39 AM
> > To: dev@dpdk.org
> > Cc: Ziye Yang <optimistyzy@gmail.com>
> > Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak
> > issue of logid
> >
> > 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.
> >
> > Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal.c     | 21 ++++++++++++++++++++-
> >  lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++++++++++-
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > b/lib/librte_eal/linuxapp/eal/eal.c
> > index e59ac65..e150200 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -793,7 +793,7 @@ 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;
> > +	char *logid;
> >  	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> >  	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> >
> > @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
> >
> >  	logid = strrchr(argv[0], '/');
> >  	logid = strdup(logid ? logid + 1: argv[0]);
> > +	if (!logid) {
> > +		rte_eal_init_alert("Cannot allocate memory for logid\n");
> > +		rte_errno = ENOMEM;
> > +		rte_atomic32_clear(&run_once);
> > +		return -1;
> > +	}
> >
> >  	thread_id = pthread_self();
> >
> > @@ -823,6 +829,8 @@ 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);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Invalid 'command line' arguments.");
> >  		rte_errno = EINVAL;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot init plugins\n");
> >  		rte_errno = EINVAL;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> >  	if (eal_option_device_parse()) {
> >  		rte_errno = ENODEV;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -851,6 +862,8 @@ 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);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -861,6 +874,8 @@ 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);
> > +			free(logid);
> >  			return -1;
> >  		}
> >  	}
> > @@ -869,6 +884,7 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot scan the buses for devices\n");
> >  		rte_errno = ENODEV;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -893,6 +909,7 @@ static void rte_eal_init_alert(const char *msg)
> >  			rte_eal_init_alert("Cannot get hugepage information.");
> >  			rte_errno = EACCES;
> >  			rte_atomic32_clear(&run_once);
> > +			free(logid);
> >  			return -1;
> >  		}
> >  	}
> > @@ -919,8 +936,10 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot init logging.");
> >  		rte_errno = ENOMEM;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> > +	free(logid);
> >
> >  #ifdef VFIO_PRESENT
> >  	if (rte_eal_vfio_setup() < 0) {
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c
> > b/lib/librte_eal/linuxapp/eal/eal_log.c
> > index 9d02ddd..076b1b9 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_log.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_log.c
> > @@ -19,6 +19,9 @@
> >
> >  #include "eal_private.h"
> >
> > +#define DEFAULT_LOG_ID_LEN 128
> > +char g_logid[DEFAULT_LOG_ID_LEN];
> > +
> >  /*
> >   * default log function
> >   */
> > @@ -49,12 +52,18 @@
> >  rte_eal_log_init(const char *id, int facility)  {
> >  	FILE *log_stream;
> > +	int str_len;
> >
> >  	log_stream = fopencookie(NULL, "w+", console_log_func);
> >  	if (log_stream == NULL)
> >  		return -1;
> >
> > -	openlog(id, LOG_NDELAY | LOG_PID, facility);
> > +	str_len = strlen(id);
> > +	if (str_len >= DEFAULT_LOG_ID_LEN)
> > +		return -1;
> > +
> > +	strncpy(g_logid, id, str_len);
> > +	openlog(g_logid, LOG_NDELAY | LOG_PID, facility);
> 
> 
> Could you explain what is the point here to create a global here and pass pointer to it to openlog()?
> Konstantin
> 
> >
> >  	eal_log_set_default(log_stream);
> >
> > --
> > 1.9.3
Ziye Yang Sept. 10, 2018, 12:23 p.m. UTC | #4 
Hi Konstantin,

Thanks for your comments.  I will revise the patch and submit another patch later.



Best Regards
Ziye Yang 


-----Original Message-----
From: Ananyev, Konstantin 
Sent: Monday, September 10, 2018 8:19 PM
To: Yang, Ziye <ziye.yang@intel.com>; dev@dpdk.org
Cc: Ziye Yang <optimistyzy@gmail.com>
Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid



> -----Original Message-----
> From: Yang, Ziye
> Sent: Monday, September 10, 2018 12:58 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak 
> issue of logid
> 
> Hi Konstantin,
> 
> The global logid is used to prevent the memory leak. If we do not do 
> this, but directly free(logid), this pointer will still be used by 
> openlog, which means that we cannot free the logid anymore. And by adding the global variable, it will not only solve the memory leak but also makes the openlog function still work.

But then I suppose you can avoid any dynamic memory allocation at all with something like that:
rte_eal_init(int argc, char **argv)
{
   ...
   static char logid[PATH_MAX];
   ...
   const char *p= strrchr(argv[0], '/');
   snprintf(logid, sizeof(logid), "%s", p);
   ...
   rte_eal_log_init(logid, ...);

?
Konstantin

> 
> 
> Best Regards
> Ziye Yang
> 
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, September 10, 2018 7:55 PM
> To: Yang, Ziye <ziye.yang@intel.com>; dev@dpdk.org
> Cc: Ziye Yang <optimistyzy@gmail.com>
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak 
> issue of logid
> 
> Hi
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ziye Yang
> > Sent: Wednesday, September 5, 2018 6:39 AM
> > To: dev@dpdk.org
> > Cc: Ziye Yang <optimistyzy@gmail.com>
> > Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak 
> > issue of logid
> >
> > 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.
> >
> > Signed-off-by: Ziye Yang <ziye.yang@intel.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal.c     | 21 ++++++++++++++++++++-
> >  lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++++++++++-
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > b/lib/librte_eal/linuxapp/eal/eal.c
> > index e59ac65..e150200 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -793,7 +793,7 @@ 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;
> > +	char *logid;
> >  	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> >  	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> >
> > @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
> >
> >  	logid = strrchr(argv[0], '/');
> >  	logid = strdup(logid ? logid + 1: argv[0]);
> > +	if (!logid) {
> > +		rte_eal_init_alert("Cannot allocate memory for logid\n");
> > +		rte_errno = ENOMEM;
> > +		rte_atomic32_clear(&run_once);
> > +		return -1;
> > +	}
> >
> >  	thread_id = pthread_self();
> >
> > @@ -823,6 +829,8 @@ 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);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Invalid 'command line' arguments.");
> >  		rte_errno = EINVAL;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot init plugins\n");
> >  		rte_errno = EINVAL;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> >  	if (eal_option_device_parse()) {
> >  		rte_errno = ENODEV;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -851,6 +862,8 @@ 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);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -861,6 +874,8 @@ 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);
> > +			free(logid);
> >  			return -1;
> >  		}
> >  	}
> > @@ -869,6 +884,7 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot scan the buses for devices\n");
> >  		rte_errno = ENODEV;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> >
> > @@ -893,6 +909,7 @@ static void rte_eal_init_alert(const char *msg)
> >  			rte_eal_init_alert("Cannot get hugepage information.");
> >  			rte_errno = EACCES;
> >  			rte_atomic32_clear(&run_once);
> > +			free(logid);
> >  			return -1;
> >  		}
> >  	}
> > @@ -919,8 +936,10 @@ static void rte_eal_init_alert(const char *msg)
> >  		rte_eal_init_alert("Cannot init logging.");
> >  		rte_errno = ENOMEM;
> >  		rte_atomic32_clear(&run_once);
> > +		free(logid);
> >  		return -1;
> >  	}
> > +	free(logid);
> >
> >  #ifdef VFIO_PRESENT
> >  	if (rte_eal_vfio_setup() < 0) {
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c
> > b/lib/librte_eal/linuxapp/eal/eal_log.c
> > index 9d02ddd..076b1b9 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_log.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_log.c
> > @@ -19,6 +19,9 @@
> >
> >  #include "eal_private.h"
> >
> > +#define DEFAULT_LOG_ID_LEN 128
> > +char g_logid[DEFAULT_LOG_ID_LEN];
> > +
> >  /*
> >   * default log function
> >   */
> > @@ -49,12 +52,18 @@
> >  rte_eal_log_init(const char *id, int facility)  {
> >  	FILE *log_stream;
> > +	int str_len;
> >
> >  	log_stream = fopencookie(NULL, "w+", console_log_func);
> >  	if (log_stream == NULL)
> >  		return -1;
> >
> > -	openlog(id, LOG_NDELAY | LOG_PID, facility);
> > +	str_len = strlen(id);
> > +	if (str_len >= DEFAULT_LOG_ID_LEN)
> > +		return -1;
> > +
> > +	strncpy(g_logid, id, str_len);
> > +	openlog(g_logid, LOG_NDELAY | LOG_PID, facility);
> 
> 
> Could you explain what is the point here to create a global here and pass pointer to it to openlog()?
> Konstantin
> 
> >
> >  	eal_log_set_default(log_stream);
> >
> > --
> > 1.9.3
diff mbox series

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index e59ac65..e150200 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -793,7 +793,7 @@  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;
+	char *logid;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
@@ -812,6 +812,12 @@  static void rte_eal_init_alert(const char *msg)
 
 	logid = strrchr(argv[0], '/');
 	logid = strdup(logid ? logid + 1: argv[0]);
+	if (!logid) {
+		rte_eal_init_alert("Cannot allocate memory for logid\n");
+		rte_errno = ENOMEM;
+		rte_atomic32_clear(&run_once);
+		return -1;
+	}
 
 	thread_id = pthread_self();
 
@@ -823,6 +829,8 @@  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);
+		free(logid);
 		return -1;
 	}
 
@@ -831,6 +839,7 @@  static void rte_eal_init_alert(const char *msg)
 		rte_eal_init_alert("Invalid 'command line' arguments.");
 		rte_errno = EINVAL;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -838,12 +847,14 @@  static void rte_eal_init_alert(const char *msg)
 		rte_eal_init_alert("Cannot init plugins\n");
 		rte_errno = EINVAL;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
 	if (eal_option_device_parse()) {
 		rte_errno = ENODEV;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -851,6 +862,8 @@  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);
+		free(logid);
 		return -1;
 	}
 
@@ -861,6 +874,8 @@  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);
+			free(logid);
 			return -1;
 		}
 	}
@@ -869,6 +884,7 @@  static void rte_eal_init_alert(const char *msg)
 		rte_eal_init_alert("Cannot scan the buses for devices\n");
 		rte_errno = ENODEV;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -893,6 +909,7 @@  static void rte_eal_init_alert(const char *msg)
 			rte_eal_init_alert("Cannot get hugepage information.");
 			rte_errno = EACCES;
 			rte_atomic32_clear(&run_once);
+			free(logid);
 			return -1;
 		}
 	}
@@ -919,8 +936,10 @@  static void rte_eal_init_alert(const char *msg)
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
+	free(logid);
 
 #ifdef VFIO_PRESENT
 	if (rte_eal_vfio_setup() < 0) {
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index 9d02ddd..076b1b9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -19,6 +19,9 @@ 
 
 #include "eal_private.h"
 
+#define DEFAULT_LOG_ID_LEN 128
+char g_logid[DEFAULT_LOG_ID_LEN];
+
 /*
  * default log function
  */
@@ -49,12 +52,18 @@ 
 rte_eal_log_init(const char *id, int facility)
 {
 	FILE *log_stream;
+	int str_len;
 
 	log_stream = fopencookie(NULL, "w+", console_log_func);
 	if (log_stream == NULL)
 		return -1;
 
-	openlog(id, LOG_NDELAY | LOG_PID, facility);
+	str_len = strlen(id);
+	if (str_len >= DEFAULT_LOG_ID_LEN)
+		return -1;
+
+	strncpy(g_logid, id, str_len);
+	openlog(g_logid, LOG_NDELAY | LOG_PID, facility);
 
 	eal_log_set_default(log_stream);