DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
@ 2018-09-05  5:39 Ziye Yang
  2018-09-10 11:54 ` Ananyev, Konstantin
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Ziye Yang @ 2018-09-05  5:39 UTC (permalink / raw)
  To: dev; +Cc: Ziye Yang

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);
 
 	eal_log_set_default(log_stream);
 
-- 
1.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-05  5:39 [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid Ziye Yang
@ 2018-09-10 11:54 ` Ananyev, Konstantin
  2018-09-10 11:58   ` Yang, Ziye
  2018-09-10 13:46 ` [dpdk-dev] [PATCH v6] " Ziye Yang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2018-09-10 11:54 UTC (permalink / raw)
  To: Yang, Ziye, dev; +Cc: Ziye Yang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-10 11:54 ` Ananyev, Konstantin
@ 2018-09-10 11:58   ` Yang, Ziye
  2018-09-10 12:19     ` Ananyev, Konstantin
  0 siblings, 1 reply; 16+ messages in thread
From: Yang, Ziye @ 2018-09-10 11:58 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev; +Cc: Ziye Yang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-10 11:58   ` Yang, Ziye
@ 2018-09-10 12:19     ` Ananyev, Konstantin
  2018-09-10 12:23       ` Yang, Ziye
  0 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2018-09-10 12:19 UTC (permalink / raw)
  To: Yang, Ziye, dev; +Cc: Ziye Yang



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-10 12:19     ` Ananyev, Konstantin
@ 2018-09-10 12:23       ` Yang, Ziye
  0 siblings, 0 replies; 16+ messages in thread
From: Yang, Ziye @ 2018-09-10 12:23 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev; +Cc: Ziye Yang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v6] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-05  5:39 [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid Ziye Yang
  2018-09-10 11:54 ` Ananyev, Konstantin
@ 2018-09-10 13:46 ` Ziye Yang
  2018-09-11  1:27 ` [dpdk-dev] [PATCH v7] " Ziye Yang
  2018-09-12  1:31 ` [dpdk-dev] [PATCH v8] " Ziye Yang
  3 siblings, 0 replies; 16+ messages in thread
From: Ziye Yang @ 2018-09-10 13:46 UTC (permalink / raw)
  To: dev; +Cc: konstantin.ananyev, Ziye Yang

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.

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..a1b218a 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];
 	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);
 		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);
 		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);
 			return -1;
 		}
 	}
-- 
1.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-05  5:39 [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid Ziye Yang
  2018-09-10 11:54 ` Ananyev, Konstantin
  2018-09-10 13:46 ` [dpdk-dev] [PATCH v6] " Ziye Yang
@ 2018-09-11  1:27 ` Ziye Yang
  2018-09-11 10:09   ` Ananyev, Konstantin
  2018-09-11 13:47   ` Aaron Conole
  2018-09-12  1:31 ` [dpdk-dev] [PATCH v8] " Ziye Yang
  3 siblings, 2 replies; 16+ messages in thread
From: Ziye Yang @ 2018-09-11  1:27 UTC (permalink / raw)
  To: dev; +Cc: konstantin.ananyev, Ziye Yang

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.

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];
 	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);
 		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);
 		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);
 			return -1;
 		}
 	}
-- 
1.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-11  1:27 ` [dpdk-dev] [PATCH v7] " Ziye Yang
@ 2018-09-11 10:09   ` Ananyev, Konstantin
  2018-09-11 13:47   ` Aaron Conole
  1 sibling, 0 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2018-09-11 10:09 UTC (permalink / raw)
  To: Yang, Ziye, dev; +Cc: Ziye Yang



> -----Original Message-----
> From: Yang, Ziye
> Sent: Tuesday, September 11, 2018 2:28 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Ziye Yang <optimistyzy@gmail.com>
> Subject: [PATCH v7] 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.
> 
> By the way, we also fix several missed function call of
> rte_atomic32_clear.
> 
> 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];
>  	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);
>  		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);
>  		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);
>  			return -1;
>  		}
>  	}
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 1.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-11  1:27 ` [dpdk-dev] [PATCH v7] " Ziye Yang
  2018-09-11 10:09   ` Ananyev, Konstantin
@ 2018-09-11 13:47   ` Aaron Conole
  2018-09-11 14:06     ` Ananyev, Konstantin
  1 sibling, 1 reply; 16+ messages in thread
From: Aaron Conole @ 2018-09-11 13:47 UTC (permalink / raw)
  To: Ziye Yang; +Cc: dev, konstantin.ananyev, Ziye Yang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-11 13:47   ` Aaron Conole
@ 2018-09-11 14:06     ` Ananyev, Konstantin
  2018-09-11 15:27       ` Aaron Conole
  2018-09-13 13:28       ` Aaron Conole
  0 siblings, 2 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2018-09-11 14:06 UTC (permalink / raw)
  To: Aaron Conole, Yang, Ziye; +Cc: dev, Ziye Yang



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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-11 14:06     ` Ananyev, Konstantin
@ 2018-09-11 15:27       ` Aaron Conole
  2018-09-13 13:28       ` Aaron Conole
  1 sibling, 0 replies; 16+ messages in thread
From: Aaron Conole @ 2018-09-11 15:27 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: Yang, Ziye, dev, Ziye Yang

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v8] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-05  5:39 [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid Ziye Yang
                   ` (2 preceding siblings ...)
  2018-09-11  1:27 ` [dpdk-dev] [PATCH v7] " Ziye Yang
@ 2018-09-12  1:31 ` Ziye Yang
  2018-10-16 15:33   ` Ferruh Yigit
  2018-10-22  8:00   ` Thomas Monjalon
  3 siblings, 2 replies; 16+ messages in thread
From: Ziye Yang @ 2018-09-12  1:31 UTC (permalink / raw)
  To: dev; +Cc: konstantin.ananyev, Ziye Yang

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index e59ac65..46494f9 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];
 	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);
-- 
1.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v7] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-11 14:06     ` Ananyev, Konstantin
  2018-09-11 15:27       ` Aaron Conole
@ 2018-09-13 13:28       ` Aaron Conole
  1 sibling, 0 replies; 16+ messages in thread
From: Aaron Conole @ 2018-09-13 13:28 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: Yang, Ziye, dev, Ziye Yang, Thomas Monjalon

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.ananyev@intel.com> writes:

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v8] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-12  1:31 ` [dpdk-dev] [PATCH v8] " Ziye Yang
@ 2018-10-16 15:33   ` Ferruh Yigit
  2018-10-22  8:00   ` Thomas Monjalon
  1 sibling, 0 replies; 16+ messages in thread
From: Ferruh Yigit @ 2018-10-16 15:33 UTC (permalink / raw)
  To: Ziye Yang, dev; +Cc: konstantin.ananyev, Ziye Yang

On 9/12/2018 2:31 AM, Ziye Yang wrote:
> 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>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


check-git-log.sh complains about commit log, it can be:
eal/linux: fix memory leak of logid


Also needs:

Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
CC: stable@dpdk.org

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v8] linuxapp, eal: Fix the memory leak issue of logid
  2018-09-12  1:31 ` [dpdk-dev] [PATCH v8] " Ziye Yang
  2018-10-16 15:33   ` Ferruh Yigit
@ 2018-10-22  8:00   ` Thomas Monjalon
  2018-10-28 10:41     ` Thomas Monjalon
  1 sibling, 1 reply; 16+ messages in thread
From: Thomas Monjalon @ 2018-10-22  8:00 UTC (permalink / raw)
  To: Ziye Yang; +Cc: dev, konstantin.ananyev, Ziye Yang

12/09/2018 03:31, Ziye Yang:
> 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>
> ---
> -	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]));

Shouldn't it be strlcpy instead of snprintf?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v8] linuxapp, eal: Fix the memory leak issue of logid
  2018-10-22  8:00   ` Thomas Monjalon
@ 2018-10-28 10:41     ` Thomas Monjalon
  0 siblings, 0 replies; 16+ messages in thread
From: Thomas Monjalon @ 2018-10-28 10:41 UTC (permalink / raw)
  To: Ziye Yang; +Cc: dev, konstantin.ananyev, Ziye Yang, ferruh.yigit

22/10/2018 10:00, Thomas Monjalon:
> 12/09/2018 03:31, Ziye Yang:
> > 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>
> > ---
> > -	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]));
> 
> Shouldn't it be strlcpy instead of snprintf?

Applied with suggested replacement:

-       snprintf(logid, sizeof(logid), "%s", (p ? p + 1 : argv[0]));
+       strlcpy(logid, p ? p + 1 : argv[0], sizeof(logid));

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-10-28 10:41 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-05  5:39 [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid Ziye Yang
2018-09-10 11:54 ` Ananyev, Konstantin
2018-09-10 11:58   ` Yang, Ziye
2018-09-10 12:19     ` Ananyev, Konstantin
2018-09-10 12:23       ` Yang, Ziye
2018-09-10 13:46 ` [dpdk-dev] [PATCH v6] " Ziye Yang
2018-09-11  1:27 ` [dpdk-dev] [PATCH v7] " Ziye Yang
2018-09-11 10:09   ` Ananyev, Konstantin
2018-09-11 13:47   ` Aaron Conole
2018-09-11 14:06     ` Ananyev, Konstantin
2018-09-11 15:27       ` Aaron Conole
2018-09-13 13:28       ` Aaron Conole
2018-09-12  1:31 ` [dpdk-dev] [PATCH v8] " Ziye Yang
2018-10-16 15:33   ` Ferruh Yigit
2018-10-22  8:00   ` Thomas Monjalon
2018-10-28 10:41     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).