diff mbox

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

Message ID 20170922144820.16590-1-danielx.t.mrzyglod@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

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

Commit Message

Daniel Mrzyglod Sept. 22, 2017, 2:48 p.m. UTC 
  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(-)

Comments

Michal Jastrzebski Oct. 2, 2017, 2:16 p.m. UTC | #1 
> -----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.
Ferruh Yigit Oct. 4, 2017, 7:24 p.m. UTC | #2 
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;
>  	}
>  

<...>
Thomas Monjalon Oct. 5, 2017, 10:33 p.m. UTC | #3 
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].
diff mbox

Patch

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