[dpdk-dev] [PATCH] eal: fix resource leak

[dpdk-dev] [PATCH] eal: fix resource leak

[Daniel Mrzyglod]

Memory allocated in strdup is not free.

Coverity issue: 143257
Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
Cc: thomas@monjalon.net

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 48f12f4..1bdbc3f 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -781,6 +781,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_cpu_init() < 0) {
 		rte_eal_init_alert("Cannot detect lcores.");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -789,6 +790,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Invalid 'command line' arguments.");
 		rte_errno = EINVAL;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -799,6 +801,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot get hugepage information.");
 		rte_errno = EACCES;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -826,6 +829,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -834,6 +838,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot init VFIO\n");
 		rte_errno = EAGAIN;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 #endif
@@ -841,6 +846,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_memory_init() < 0) {
 		rte_eal_init_alert("Cannot init memory\n");
 		rte_errno = ENOMEM;
+		free(logid);
 		return -1;
 	}
 
@@ -850,24 +856,28 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_memzone_init() < 0) {
 		rte_eal_init_alert("Cannot init memzone\n");
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_tailqs_init() < 0) {
 		rte_eal_init_alert("Cannot init tail queues for objects\n");
 		rte_errno = EFAULT;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_alarm_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
 		/* rte_eal_alarm_init sets rte_errno on failure. */
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_timer_init() < 0) {
 		rte_eal_init_alert("Cannot init HPET or TSC timers\n");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -886,17 +896,20 @@ rte_eal_init(int argc, char **argv)
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
+		free(logid);
 		return -1;
 	}
 
 	if (eal_option_device_parse()) {
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_bus_scan()) {
 		rte_eal_init_alert("Cannot scan the buses for devices\n");
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
@@ -941,6 +954,7 @@ rte_eal_init(int argc, char **argv)
 	if (ret) {
 		rte_eal_init_alert("rte_service_init() failed\n");
 		rte_errno = ENOEXEC;
+		free(logid);
 		return -1;
 	}
 
@@ -948,6 +962,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_bus_probe()) {
 		rte_eal_init_alert("Cannot probe devices\n");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -957,6 +972,7 @@ rte_eal_init(int argc, char **argv)
 	ret = rte_service_start_with_defaults();
 	if (ret < 0 && ret != -ENOTSUP) {
 		rte_errno = ENOEXEC;
+		free(logid);
 		return -1;
 	}
 
-- 
2.7.4

[dpdk-dev] [PATCH v2] eal: fix resource leak

[Daniel Mrzyglod]

Memory allocated in strdup is not free.

Coverity issue: 143257
Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
Cc: thomas@monjalon.net

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
v2:
* Fix due to compilation errors

 lib/librte_eal/linuxapp/eal/eal.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 48f12f4..a7df566 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -751,7 +751,7 @@ rte_eal_init(int argc, char **argv)
 	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];
 
@@ -781,6 +781,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_cpu_init() < 0) {
 		rte_eal_init_alert("Cannot detect lcores.");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -789,6 +790,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Invalid 'command line' arguments.");
 		rte_errno = EINVAL;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -799,6 +801,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot get hugepage information.");
 		rte_errno = EACCES;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -826,6 +829,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 
@@ -834,6 +838,7 @@ rte_eal_init(int argc, char **argv)
 		rte_eal_init_alert("Cannot init VFIO\n");
 		rte_errno = EAGAIN;
 		rte_atomic32_clear(&run_once);
+		free(logid);
 		return -1;
 	}
 #endif
@@ -841,6 +846,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_memory_init() < 0) {
 		rte_eal_init_alert("Cannot init memory\n");
 		rte_errno = ENOMEM;
+		free(logid);
 		return -1;
 	}
 
@@ -850,24 +856,28 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_memzone_init() < 0) {
 		rte_eal_init_alert("Cannot init memzone\n");
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_tailqs_init() < 0) {
 		rte_eal_init_alert("Cannot init tail queues for objects\n");
 		rte_errno = EFAULT;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_alarm_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
 		/* rte_eal_alarm_init sets rte_errno on failure. */
+		free(logid);
 		return -1;
 	}
 
 	if (rte_eal_timer_init() < 0) {
 		rte_eal_init_alert("Cannot init HPET or TSC timers\n");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -886,17 +896,20 @@ rte_eal_init(int argc, char **argv)
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
+		free(logid);
 		return -1;
 	}
 
 	if (eal_option_device_parse()) {
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
 	if (rte_bus_scan()) {
 		rte_eal_init_alert("Cannot scan the buses for devices\n");
 		rte_errno = ENODEV;
+		free(logid);
 		return -1;
 	}
 
@@ -941,6 +954,7 @@ rte_eal_init(int argc, char **argv)
 	if (ret) {
 		rte_eal_init_alert("rte_service_init() failed\n");
 		rte_errno = ENOEXEC;
+		free(logid);
 		return -1;
 	}
 
@@ -948,6 +962,7 @@ rte_eal_init(int argc, char **argv)
 	if (rte_bus_probe()) {
 		rte_eal_init_alert("Cannot probe devices\n");
 		rte_errno = ENOTSUP;
+		free(logid);
 		return -1;
 	}
 
@@ -957,6 +972,7 @@ rte_eal_init(int argc, char **argv)
 	ret = rte_service_start_with_defaults();
 	if (ret < 0 && ret != -ENOTSUP) {
 		rte_errno = ENOEXEC;
+		free(logid);
 		return -1;
 	}
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] eal: fix resource leak

[Jastrzebski, MichalX K]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Daniel Mrzyglod
> Sent: Friday, September 22, 2017 4:48 PM
> To: thomas@monjalon.net
> Cc: dev@dpdk.org; Mrzyglod, DanielX T <danielx.t.mrzyglod@intel.com>
> Subject: [dpdk-dev] [PATCH v2] eal: fix resource leak
> 
> Memory allocated in strdup is not free.
> 
> Coverity issue: 143257
> Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
> Cc: thomas@monjalon.net
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
> v2:
> * Fix due to compilation errors
> 
>  lib/librte_eal/linuxapp/eal/eal.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 48f12f4..a7df566 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -751,7 +751,7 @@ rte_eal_init(int argc, char **argv)
>  	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];
> 
> @@ -781,6 +781,7 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_cpu_init() < 0) {
>  		rte_eal_init_alert("Cannot detect lcores.");
>  		rte_errno = ENOTSUP;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -789,6 +790,7 @@ rte_eal_init(int argc, char **argv)
>  		rte_eal_init_alert("Invalid 'command line' arguments.");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -799,6 +801,7 @@ rte_eal_init(int argc, char **argv)
>  		rte_eal_init_alert("Cannot get hugepage information.");
>  		rte_errno = EACCES;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -826,6 +829,7 @@ rte_eal_init(int argc, char **argv)
>  		rte_eal_init_alert("Cannot init logging.");
>  		rte_errno = ENOMEM;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -834,6 +838,7 @@ rte_eal_init(int argc, char **argv)
>  		rte_eal_init_alert("Cannot init VFIO\n");
>  		rte_errno = EAGAIN;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
>  #endif
> @@ -841,6 +846,7 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_memory_init() < 0) {
>  		rte_eal_init_alert("Cannot init memory\n");
>  		rte_errno = ENOMEM;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -850,24 +856,28 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_memzone_init() < 0) {
>  		rte_eal_init_alert("Cannot init memzone\n");
>  		rte_errno = ENODEV;
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (rte_eal_tailqs_init() < 0) {
>  		rte_eal_init_alert("Cannot init tail queues for objects\n");
>  		rte_errno = EFAULT;
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (rte_eal_alarm_init() < 0) {
>  		rte_eal_init_alert("Cannot init interrupt-handling
> thread\n");
>  		/* rte_eal_alarm_init sets rte_errno on failure. */
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (rte_eal_timer_init() < 0) {
>  		rte_eal_init_alert("Cannot init HPET or TSC timers\n");
>  		rte_errno = ENOTSUP;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -886,17 +896,20 @@ rte_eal_init(int argc, char **argv)
> 
>  	if (rte_eal_intr_init() < 0) {
>  		rte_eal_init_alert("Cannot init interrupt-handling
> thread\n");
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (eal_option_device_parse()) {
>  		rte_errno = ENODEV;
> +		free(logid);
>  		return -1;
>  	}
> 
>  	if (rte_bus_scan()) {
>  		rte_eal_init_alert("Cannot scan the buses for devices\n");
>  		rte_errno = ENODEV;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -941,6 +954,7 @@ rte_eal_init(int argc, char **argv)
>  	if (ret) {
>  		rte_eal_init_alert("rte_service_init() failed\n");
>  		rte_errno = ENOEXEC;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -948,6 +962,7 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_bus_probe()) {
>  		rte_eal_init_alert("Cannot probe devices\n");
>  		rte_errno = ENOTSUP;
> +		free(logid);
>  		return -1;
>  	}
> 
> @@ -957,6 +972,7 @@ rte_eal_init(int argc, char **argv)
>  	ret = rte_service_start_with_defaults();
>  	if (ret < 0 && ret != -ENOTSUP) {
>  		rte_errno = ENOEXEC;
> +		free(logid);
>  		return -1;
>  	}
> 
> --
> 2.7.4

Hi Thomas,
I would like to ask for a feedback regarding proposed fix.
If everything is ok with it, please send acked-by.

Best regards
Michal.

Re: [dpdk-dev] [PATCH v2] eal: fix resource leak

[Ferruh Yigit]

On 9/22/2017 3:48 PM, Daniel Mrzyglod wrote:
> Memory allocated in strdup is not free.
> 
> Coverity issue: 143257
> Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
> Cc: thomas@monjalon.net
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
> v2:
> * Fix due to compilation errors
> 
>  lib/librte_eal/linuxapp/eal/eal.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 48f12f4..a7df566 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -751,7 +751,7 @@ rte_eal_init(int argc, char **argv)
>  	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];
>  
> @@ -781,6 +781,7 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_cpu_init() < 0) {
>  		rte_eal_init_alert("Cannot detect lcores.");
>  		rte_errno = ENOTSUP;
> +		free(logid);

Hi Daniel,

This works but this variable is a nuance and adding free() for this it
into main eal features fail path looks like noise.

Initially, do we need to strdup this variable at all?
What will happen if logid fed into rte_eal_log_init() without strdup?
Since it is const char *, I guess the string is just for read and
content won't be changed so it should be OK I guess.

If above is not right, what about creating a static variable and use it
instead of dynamically allocating the logid, what do you think?

Thanks,
ferruh

>  		return -1;
>  	}
>  
> @@ -789,6 +790,7 @@ rte_eal_init(int argc, char **argv)
>  		rte_eal_init_alert("Invalid 'command line' arguments.");
>  		rte_errno = EINVAL;
>  		rte_atomic32_clear(&run_once);
> +		free(logid);
>  		return -1;
>  	}
>  

<...>

Re: [dpdk-dev] [PATCH v2] eal: fix resource leak

[Thomas Monjalon]

04/10/2017 21:24, Ferruh Yigit:
> On 9/22/2017 3:48 PM, Daniel Mrzyglod wrote:
> > Memory allocated in strdup is not free.
> > 
> > Coverity issue: 143257
> > Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
> > Cc: thomas@monjalon.net
> > 
> > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> > ---
> This works but this variable is a nuance and adding free() for this it
> into main eal features fail path looks like noise.
> 
> Initially, do we need to strdup this variable at all?
> What will happen if logid fed into rte_eal_log_init() without strdup?
> Since it is const char *, I guess the string is just for read and
> content won't be changed so it should be OK I guess.
> 
> If above is not right, what about creating a static variable and use it
> instead of dynamically allocating the logid, what do you think?

Good proposal Ferruh.
It seems strdup is not needed as it is basically argv[0].

[dpdk-dev] [PATCH v3] eal: fix resource leak

[Daniel Mrzyglod]

Memory allocated in strdup is not free.

Coverity issue: 143257
Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
Cc: thomas@monjalon.net
Cc: stable@dpdk.org

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
v3: 
* remove strdup because it's basically striped argv[0]
v2:
* Fix due to compilation errors
 
 lib/librte_eal/linuxapp/eal/eal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 1f07347..739b61a 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -763,7 +763,7 @@ rte_eal_init(int argc, char **argv)
 	}
 
 	logid = strrchr(argv[0], '/');
-	logid = strdup(logid ? logid + 1: argv[0]);
+	logid = logid ? logid + 1 : argv[0];
 
 	thread_id = pthread_self();
 
-- 
2.7.4

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] eal: fix resource leak

[Thomas Monjalon]

11/10/2017 13:53, Daniel Mrzyglod:
> Memory allocated in strdup is not free.
> 
> Coverity issue: 143257
> Fixes: d8a2bc71dfc2 ("log: remove app path from syslog id")
> Cc: thomas@monjalon.net
> Cc: stable@dpdk.org
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>

Given this recent commit:
	http://dpdk.org/commit/e3f141879ef
I think we should consider this issue as a false positive.

The arguments given to rte_eal_init may be freed.
So it's better to strdup them, even if it is never freed.