patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/softnic: fix memory leak in parsing arguments
@ 2021-07-08  8:44 dapengx.yu
  2021-07-13  8:44 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
  2021-07-13 10:08 ` [dpdk-stable] [PATCH v2] " dapengx.yu
  0 siblings, 2 replies; 10+ messages in thread
From: dapengx.yu @ 2021-07-08  8:44 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dapeng Yu, stable

From: Dapeng Yu <dapengx.yu@intel.com>

In function pmd_parse_args(), firmware path is duplicated from device
arguments as character string, but is never freed, which cause memory
leak.

This patch changes the type of firmware member of struct pmd_params to
character array, to make memory resource release unnecessary, and
changes the type of name member to character array, to keep the
consistency of character string handling in struct pmd_params.

Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
Cc: stable@dpdk.org

Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
 drivers/net/softnic/rte_eth_softnic.c           | 9 ++++++---
 drivers/net/softnic/rte_eth_softnic_internals.h | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index f64023256d..4d2f93e9c6 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 {
 	struct rte_kvargs *kvlist;
 	int ret = 0;
+	char *firmware = NULL;
 
 	kvlist = rte_kvargs_parse(params, pmd_valid_args);
 	if (kvlist == NULL)
@@ -447,7 +448,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 
 	/* Set default values */
 	memset(p, 0, sizeof(*p));
-	p->firmware = SOFTNIC_FIRMWARE;
+	snprintf(p->firmware, sizeof(p->firmware), "%s", SOFTNIC_FIRMWARE);
 	p->cpu_id = SOFTNIC_CPU_ID;
 	p->sc = SOFTNIC_SC;
 	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
@@ -468,10 +469,12 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 	/* Firmware script (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
-			&get_string, &p->firmware);
+			&get_string, &firmware);
 		if (ret < 0)
 			goto out_free;
 	}
+	snprintf(p->firmware, sizeof(p->firmware), "%s", firmware);
+	free(firmware);
 
 	/* Connection listening port (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
@@ -621,7 +624,7 @@ pmd_probe(struct rte_vdev_device *vdev)
 	if (status)
 		return status;
 
-	p.name = name;
+	snprintf(p.name, sizeof(p.name), "%s", name);
 
 	/* Allocate and initialize soft ethdev private data */
 	dev_private = pmd_init(&p);
diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
index 1b3186ef0b..a13d67b7c1 100644
--- a/drivers/net/softnic/rte_eth_softnic_internals.h
+++ b/drivers/net/softnic/rte_eth_softnic_internals.h
@@ -34,8 +34,8 @@
  */
 
 struct pmd_params {
-	const char *name;
-	const char *firmware;
+	char name[RTE_DEV_NAME_MAX_LEN];
+	char firmware[PATH_MAX];
 	uint16_t conn_port;
 	uint32_t cpu_id;
 	int sc; /**< Service cores. */
-- 
2.27.0


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/softnic: fix memory leak in parsing arguments
  2021-07-08  8:44 [dpdk-stable] [PATCH] net/softnic: fix memory leak in parsing arguments dapengx.yu
@ 2021-07-13  8:44 ` Andrew Rybchenko
  2021-07-13 10:08 ` [dpdk-stable] [PATCH v2] " dapengx.yu
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Rybchenko @ 2021-07-13  8:44 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu, dapengx.yu; +Cc: dev, stable

@Jasvinder, @Cristian, could you the patch, please.

On 7/8/21 11:44 AM, dapengx.yu@intel.com wrote:
> From: Dapeng Yu <dapengx.yu@intel.com>
> 
> In function pmd_parse_args(), firmware path is duplicated from device
> arguments as character string, but is never freed, which cause memory
> leak.
> 
> This patch changes the type of firmware member of struct pmd_params to
> character array, to make memory resource release unnecessary, and
> changes the type of name member to character array, to keep the
> consistency of character string handling in struct pmd_params.
> 
> Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---
>  drivers/net/softnic/rte_eth_softnic.c           | 9 ++++++---
>  drivers/net/softnic/rte_eth_softnic_internals.h | 4 ++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
> index f64023256d..4d2f93e9c6 100644
> --- a/drivers/net/softnic/rte_eth_softnic.c
> +++ b/drivers/net/softnic/rte_eth_softnic.c
> @@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  {
>  	struct rte_kvargs *kvlist;
>  	int ret = 0;
> +	char *firmware = NULL;
>  
>  	kvlist = rte_kvargs_parse(params, pmd_valid_args);
>  	if (kvlist == NULL)
> @@ -447,7 +448,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  
>  	/* Set default values */
>  	memset(p, 0, sizeof(*p));
> -	p->firmware = SOFTNIC_FIRMWARE;
> +	snprintf(p->firmware, sizeof(p->firmware), "%s", SOFTNIC_FIRMWARE);

Looking at strlcpy(), rte_strlcpy() and rte_strscpy() I think
that rte_strscpy() is the best option here since it is easier
to check return value to protect against buffer overflow and
shortened result.

>  	p->cpu_id = SOFTNIC_CPU_ID;
>  	p->sc = SOFTNIC_SC;
>  	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
> @@ -468,10 +469,12 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  	/* Firmware script (optional) */
>  	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
>  		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
> -			&get_string, &p->firmware);
> +			&get_string, &firmware);
>  		if (ret < 0)
>  			goto out_free;
>  	}
> +	snprintf(p->firmware, sizeof(p->firmware), "%s", firmware);

Same here.

> +	free(firmware);
>  
>  	/* Connection listening port (optional) */
>  	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
> @@ -621,7 +624,7 @@ pmd_probe(struct rte_vdev_device *vdev)
>  	if (status)
>  		return status;
>  
> -	p.name = name;
> +	snprintf(p.name, sizeof(p.name), "%s", name);

Same here.

>  
>  	/* Allocate and initialize soft ethdev private data */
>  	dev_private = pmd_init(&p);
> diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
> index 1b3186ef0b..a13d67b7c1 100644
> --- a/drivers/net/softnic/rte_eth_softnic_internals.h
> +++ b/drivers/net/softnic/rte_eth_softnic_internals.h
> @@ -34,8 +34,8 @@
>   */
>  
>  struct pmd_params {
> -	const char *name;
> -	const char *firmware;
> +	char name[RTE_DEV_NAME_MAX_LEN];
> +	char firmware[PATH_MAX];
>  	uint16_t conn_port;
>  	uint32_t cpu_id;
>  	int sc; /**< Service cores. */
> 


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

* [dpdk-stable] [PATCH v2] net/softnic: fix memory leak in parsing arguments
  2021-07-08  8:44 [dpdk-stable] [PATCH] net/softnic: fix memory leak in parsing arguments dapengx.yu
  2021-07-13  8:44 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
@ 2021-07-13 10:08 ` dapengx.yu
  2021-07-13 10:10   ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
  2021-07-14  5:47   ` [dpdk-stable] [PATCH v3] " dapengx.yu
  1 sibling, 2 replies; 10+ messages in thread
From: dapengx.yu @ 2021-07-13 10:08 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dapeng Yu, stable

From: Dapeng Yu <dapengx.yu@intel.com>

In function pmd_parse_args(), firmware path is duplicated from device
arguments as character string, but is never freed, which cause memory
leak.

This patch changes the type of firmware member of struct pmd_params to
character array, to make memory resource release unnecessary, and
changes the type of name member to character array, to keep the
consistency of character string handling in struct pmd_params.

Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
Cc: stable@dpdk.org

Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
V2:
* improve the patch according to maintainer's comment:
  rte_strscpy() is the best option here.
---
 drivers/net/softnic/rte_eth_softnic.c           | 9 ++++++---
 drivers/net/softnic/rte_eth_softnic_internals.h | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index f64023256d..8a49e83dce 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 {
 	struct rte_kvargs *kvlist;
 	int ret = 0;
+	char *firmware = NULL;
 
 	kvlist = rte_kvargs_parse(params, pmd_valid_args);
 	if (kvlist == NULL)
@@ -447,7 +448,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 
 	/* Set default values */
 	memset(p, 0, sizeof(*p));
-	p->firmware = SOFTNIC_FIRMWARE;
+	rte_strscpy(p->firmware, SOFTNIC_FIRMWARE, sizeof(p->firmware));
 	p->cpu_id = SOFTNIC_CPU_ID;
 	p->sc = SOFTNIC_SC;
 	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
@@ -468,10 +469,12 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 	/* Firmware script (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
-			&get_string, &p->firmware);
+			&get_string, &firmware);
 		if (ret < 0)
 			goto out_free;
 	}
+	rte_strscpy(p->firmware, firmware, sizeof(p->firmware));
+	free(firmware);
 
 	/* Connection listening port (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
@@ -621,7 +624,7 @@ pmd_probe(struct rte_vdev_device *vdev)
 	if (status)
 		return status;
 
-	p.name = name;
+	rte_strscpy(p.name, name, sizeof(p.name));
 
 	/* Allocate and initialize soft ethdev private data */
 	dev_private = pmd_init(&p);
diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
index 1b3186ef0b..a13d67b7c1 100644
--- a/drivers/net/softnic/rte_eth_softnic_internals.h
+++ b/drivers/net/softnic/rte_eth_softnic_internals.h
@@ -34,8 +34,8 @@
  */
 
 struct pmd_params {
-	const char *name;
-	const char *firmware;
+	char name[RTE_DEV_NAME_MAX_LEN];
+	char firmware[PATH_MAX];
 	uint16_t conn_port;
 	uint32_t cpu_id;
 	int sc; /**< Service cores. */
-- 
2.27.0


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/softnic: fix memory leak in parsing arguments
  2021-07-13 10:08 ` [dpdk-stable] [PATCH v2] " dapengx.yu
@ 2021-07-13 10:10   ` Andrew Rybchenko
  2021-07-14  5:47   ` [dpdk-stable] [PATCH v3] " dapengx.yu
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Rybchenko @ 2021-07-13 10:10 UTC (permalink / raw)
  To: dapengx.yu, Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, stable

On 7/13/21 1:08 PM, dapengx.yu@intel.com wrote:
> From: Dapeng Yu <dapengx.yu@intel.com>
> 
> In function pmd_parse_args(), firmware path is duplicated from device
> arguments as character string, but is never freed, which cause memory
> leak.
> 
> This patch changes the type of firmware member of struct pmd_params to
> character array, to make memory resource release unnecessary, and
> changes the type of name member to character array, to keep the
> consistency of character string handling in struct pmd_params.
> 
> Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---
> V2:
> * improve the patch according to maintainer's comment:
>   rte_strscpy() is the best option here.
> ---
>  drivers/net/softnic/rte_eth_softnic.c           | 9 ++++++---
>  drivers/net/softnic/rte_eth_softnic_internals.h | 4 ++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
> index f64023256d..8a49e83dce 100644
> --- a/drivers/net/softnic/rte_eth_softnic.c
> +++ b/drivers/net/softnic/rte_eth_softnic.c
> @@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  {
>  	struct rte_kvargs *kvlist;
>  	int ret = 0;
> +	char *firmware = NULL;
>  
>  	kvlist = rte_kvargs_parse(params, pmd_valid_args);
>  	if (kvlist == NULL)
> @@ -447,7 +448,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  
>  	/* Set default values */
>  	memset(p, 0, sizeof(*p));
> -	p->firmware = SOFTNIC_FIRMWARE;
> +	rte_strscpy(p->firmware, SOFTNIC_FIRMWARE, sizeof(p->firmware));

I still don't understand why return value is not checked here
and in similar cases below.

>  	p->cpu_id = SOFTNIC_CPU_ID;
>  	p->sc = SOFTNIC_SC;
>  	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
> @@ -468,10 +469,12 @@ pmd_parse_args(struct pmd_params *p, const char *params)
>  	/* Firmware script (optional) */
>  	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
>  		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
> -			&get_string, &p->firmware);
> +			&get_string, &firmware);
>  		if (ret < 0)
>  			goto out_free;
>  	}
> +	rte_strscpy(p->firmware, firmware, sizeof(p->firmware));
> +	free(firmware);
>  
>  	/* Connection listening port (optional) */
>  	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
> @@ -621,7 +624,7 @@ pmd_probe(struct rte_vdev_device *vdev)
>  	if (status)
>  		return status;
>  
> -	p.name = name;
> +	rte_strscpy(p.name, name, sizeof(p.name));
>  
>  	/* Allocate and initialize soft ethdev private data */
>  	dev_private = pmd_init(&p);
> diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
> index 1b3186ef0b..a13d67b7c1 100644
> --- a/drivers/net/softnic/rte_eth_softnic_internals.h
> +++ b/drivers/net/softnic/rte_eth_softnic_internals.h
> @@ -34,8 +34,8 @@
>   */
>  
>  struct pmd_params {
> -	const char *name;
> -	const char *firmware;
> +	char name[RTE_DEV_NAME_MAX_LEN];
> +	char firmware[PATH_MAX];
>  	uint16_t conn_port;
>  	uint32_t cpu_id;
>  	int sc; /**< Service cores. */
> 


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

* [dpdk-stable] [PATCH v3] net/softnic: fix memory leak in parsing arguments
  2021-07-13 10:08 ` [dpdk-stable] [PATCH v2] " dapengx.yu
  2021-07-13 10:10   ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
@ 2021-07-14  5:47   ` dapengx.yu
  2021-07-14 11:07     ` Singh, Jasvinder
  2021-07-15  5:38     ` [dpdk-stable] [PATCH v4] " dapengx.yu
  1 sibling, 2 replies; 10+ messages in thread
From: dapengx.yu @ 2021-07-14  5:47 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dapeng Yu, stable

From: Dapeng Yu <dapengx.yu@intel.com>

In function pmd_parse_args(), firmware path is duplicated from device
arguments as character string, but is never freed, which cause memory
leak.

This patch changes the type of firmware member of struct pmd_params to
character array, to make memory resource release unnecessary, and
changes the type of name member to character array, to keep the
consistency of character string handling in struct pmd_params.

Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
Cc: stable@dpdk.org

Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
V2:
* improve the patch according to maintainer's comment:
  rte_strscpy() is the best option here.
V3:
* improve the handling of string copy, to make the code more robust
---
 drivers/net/softnic/rte_eth_softnic.c         | 30 ++++++++++++++++---
 .../net/softnic/rte_eth_softnic_internals.h   |  4 +--
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index f64023256d..0aa7147b13 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 {
 	struct rte_kvargs *kvlist;
 	int ret = 0;
+	char *firmware = NULL;
 
 	kvlist = rte_kvargs_parse(params, pmd_valid_args);
 	if (kvlist == NULL)
@@ -447,7 +448,14 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 
 	/* Set default values */
 	memset(p, 0, sizeof(*p));
-	p->firmware = SOFTNIC_FIRMWARE;
+	if (rte_strscpy(p->firmware, SOFTNIC_FIRMWARE,
+			sizeof(p->firmware)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": firmware path should be shorter than %zu",
+			SOFTNIC_FIRMWARE, sizeof(p->firmware));
+		ret = -EINVAL;
+		goto out_free;
+	}
 	p->cpu_id = SOFTNIC_CPU_ID;
 	p->sc = SOFTNIC_SC;
 	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
@@ -468,11 +476,20 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 	/* Firmware script (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
-			&get_string, &p->firmware);
+			&get_string, &firmware);
 		if (ret < 0)
 			goto out_free;
 	}
-
+	if (rte_strscpy(p->firmware, firmware,
+			sizeof(p->firmware)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": firmware path should be shorter than %zu",
+			firmware, sizeof(p->firmware));
+		free(firmware);
+		ret = -EINVAL;
+		goto out_free;
+	}
+	free(firmware);
 	/* Connection listening port (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_CONN_PORT,
@@ -621,7 +638,12 @@ pmd_probe(struct rte_vdev_device *vdev)
 	if (status)
 		return status;
 
-	p.name = name;
+	if (rte_strscpy(p.name, name, sizeof(p.name)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": device name should be shorter than %zu",
+			name, sizeof(p.name));
+		return -EINVAL;
+	}
 
 	/* Allocate and initialize soft ethdev private data */
 	dev_private = pmd_init(&p);
diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
index 1b3186ef0b..a13d67b7c1 100644
--- a/drivers/net/softnic/rte_eth_softnic_internals.h
+++ b/drivers/net/softnic/rte_eth_softnic_internals.h
@@ -34,8 +34,8 @@
  */
 
 struct pmd_params {
-	const char *name;
-	const char *firmware;
+	char name[RTE_DEV_NAME_MAX_LEN];
+	char firmware[PATH_MAX];
 	uint16_t conn_port;
 	uint32_t cpu_id;
 	int sc; /**< Service cores. */
-- 
2.27.0


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

* Re: [dpdk-stable] [PATCH v3] net/softnic: fix memory leak in parsing arguments
  2021-07-14  5:47   ` [dpdk-stable] [PATCH v3] " dapengx.yu
@ 2021-07-14 11:07     ` Singh, Jasvinder
  2021-07-15  1:42       ` Yu, DapengX
  2021-07-15  5:38     ` [dpdk-stable] [PATCH v4] " dapengx.yu
  1 sibling, 1 reply; 10+ messages in thread
From: Singh, Jasvinder @ 2021-07-14 11:07 UTC (permalink / raw)
  To: Yu, DapengX, Dumitrescu, Cristian; +Cc: dev, stable

<snip>

> +	free(firmware);

Memory for firmware is not allocated dynamically, so no need for this.

<snip>

>  struct pmd_params {
> -	const char *name;
> -	const char *firmware;
> +	char name[RTE_DEV_NAME_MAX_LEN];

Please replace " RTE_DEV_NAME_MAX_LEN " with "NAME_SIZE" which is already defined in softnic_internals.h

> +	char firmware[PATH_MAX];

Also, instead of using PATH_MAX, define new macro "SOFTNIC_PATH_MAX   4096" in softnic_internals.h


>  	uint16_t conn_port;
>  	uint32_t cpu_id;
>  	int sc; /**< Service cores. */
> --
> 2.27.0


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

* Re: [dpdk-stable] [PATCH v3] net/softnic: fix memory leak in parsing arguments
  2021-07-14 11:07     ` Singh, Jasvinder
@ 2021-07-15  1:42       ` Yu, DapengX
  0 siblings, 0 replies; 10+ messages in thread
From: Yu, DapengX @ 2021-07-15  1:42 UTC (permalink / raw)
  To: Singh, Jasvinder, Dumitrescu, Cristian; +Cc: dev, stable



> -----Original Message-----
> From: Singh, Jasvinder <jasvinder.singh@intel.com>
> Sent: Wednesday, July 14, 2021 7:08 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH v3] net/softnic: fix memory leak in parsing arguments
> 
> <snip>
> 
> > +	free(firmware);
> 
> Memory for firmware is not allocated dynamically, so no need for this.

I have double checked it, at this code snippet, the memory block referenced by the "firmware" pointer is allocated by the strdup() in get_string() function.
The free(firmware) is necessary, and it prevents memory leak.
> 
> <snip>
> 
> >  struct pmd_params {
> > -	const char *name;
> > -	const char *firmware;
> > +	char name[RTE_DEV_NAME_MAX_LEN];
> 
> Please replace " RTE_DEV_NAME_MAX_LEN " with "NAME_SIZE" which is
> already defined in softnic_internals.h

It will be modified in the next version.
> 
> > +	char firmware[PATH_MAX];
> 
> Also, instead of using PATH_MAX, define new macro "SOFTNIC_PATH_MAX
> 4096" in softnic_internals.h

It will be modified in the next version.
> 
> 
> >  	uint16_t conn_port;
> >  	uint32_t cpu_id;
> >  	int sc; /**< Service cores. */
> > --
> > 2.27.0


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

* [dpdk-stable] [PATCH v4] net/softnic: fix memory leak in parsing arguments
  2021-07-14  5:47   ` [dpdk-stable] [PATCH v3] " dapengx.yu
  2021-07-14 11:07     ` Singh, Jasvinder
@ 2021-07-15  5:38     ` dapengx.yu
  2021-07-16 12:04       ` Singh, Jasvinder
  1 sibling, 1 reply; 10+ messages in thread
From: dapengx.yu @ 2021-07-15  5:38 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dapeng Yu, stable

From: Dapeng Yu <dapengx.yu@intel.com>

In function pmd_parse_args(), firmware path is duplicated from device
arguments as character string, but is never freed, which cause memory
leak.

This patch changes the type of firmware member of struct pmd_params to
character array, to make memory resource release unnecessary, and
changes the type of name member to character array, to keep the
consistency of character string handling in struct pmd_params.

Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
Cc: stable@dpdk.org

Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
V2:
* improve the patch according to maintainer's comment:
  rte_strscpy() is the best option here.
V3:
* improve the handling of string copy, to make the code more robust
V4:
* replace RTE_DEV_NAME_MAX_LEN with NAME_SIZE
* replace PATH_MAX with SOFTNIC_PATH_MAX
---
 drivers/net/softnic/rte_eth_softnic.c         | 30 ++++++++++++++++---
 .../net/softnic/rte_eth_softnic_internals.h   |  5 ++--
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index f64023256d..0aa7147b13 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -440,6 +440,7 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 {
 	struct rte_kvargs *kvlist;
 	int ret = 0;
+	char *firmware = NULL;
 
 	kvlist = rte_kvargs_parse(params, pmd_valid_args);
 	if (kvlist == NULL)
@@ -447,7 +448,14 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 
 	/* Set default values */
 	memset(p, 0, sizeof(*p));
-	p->firmware = SOFTNIC_FIRMWARE;
+	if (rte_strscpy(p->firmware, SOFTNIC_FIRMWARE,
+			sizeof(p->firmware)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": firmware path should be shorter than %zu",
+			SOFTNIC_FIRMWARE, sizeof(p->firmware));
+		ret = -EINVAL;
+		goto out_free;
+	}
 	p->cpu_id = SOFTNIC_CPU_ID;
 	p->sc = SOFTNIC_SC;
 	p->tm.n_queues = SOFTNIC_TM_N_QUEUES;
@@ -468,11 +476,20 @@ pmd_parse_args(struct pmd_params *p, const char *params)
 	/* Firmware script (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_FIRMWARE) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_FIRMWARE,
-			&get_string, &p->firmware);
+			&get_string, &firmware);
 		if (ret < 0)
 			goto out_free;
 	}
-
+	if (rte_strscpy(p->firmware, firmware,
+			sizeof(p->firmware)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": firmware path should be shorter than %zu",
+			firmware, sizeof(p->firmware));
+		free(firmware);
+		ret = -EINVAL;
+		goto out_free;
+	}
+	free(firmware);
 	/* Connection listening port (optional) */
 	if (rte_kvargs_count(kvlist, PMD_PARAM_CONN_PORT) == 1) {
 		ret = rte_kvargs_process(kvlist, PMD_PARAM_CONN_PORT,
@@ -621,7 +638,12 @@ pmd_probe(struct rte_vdev_device *vdev)
 	if (status)
 		return status;
 
-	p.name = name;
+	if (rte_strscpy(p.name, name, sizeof(p.name)) < 0) {
+		PMD_LOG(WARNING,
+			"\"%s\": device name should be shorter than %zu",
+			name, sizeof(p.name));
+		return -EINVAL;
+	}
 
 	/* Allocate and initialize soft ethdev private data */
 	dev_private = pmd_init(&p);
diff --git a/drivers/net/softnic/rte_eth_softnic_internals.h b/drivers/net/softnic/rte_eth_softnic_internals.h
index 1b3186ef0b..07285ca315 100644
--- a/drivers/net/softnic/rte_eth_softnic_internals.h
+++ b/drivers/net/softnic/rte_eth_softnic_internals.h
@@ -28,14 +28,15 @@
 #include "conn.h"
 
 #define NAME_SIZE                                            64
+#define SOFTNIC_PATH_MAX                                     4096
 
 /**
  * PMD Parameters
  */
 
 struct pmd_params {
-	const char *name;
-	const char *firmware;
+	char name[NAME_SIZE];
+	char firmware[SOFTNIC_PATH_MAX];
 	uint16_t conn_port;
 	uint32_t cpu_id;
 	int sc; /**< Service cores. */
-- 
2.27.0


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

* Re: [dpdk-stable] [PATCH v4] net/softnic: fix memory leak in parsing arguments
  2021-07-15  5:38     ` [dpdk-stable] [PATCH v4] " dapengx.yu
@ 2021-07-16 12:04       ` Singh, Jasvinder
  2021-07-23  8:11         ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Singh, Jasvinder @ 2021-07-16 12:04 UTC (permalink / raw)
  To: Yu, DapengX, Dumitrescu, Cristian; +Cc: dev, stable



> -----Original Message-----
> From: Yu, DapengX <dapengx.yu@intel.com>
> Sent: Thursday, July 15, 2021 6:38 AM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [PATCH v4] net/softnic: fix memory leak in parsing arguments
> 
> From: Dapeng Yu <dapengx.yu@intel.com>
> 
> In function pmd_parse_args(), firmware path is duplicated from device
> arguments as character string, but is never freed, which cause memory leak.
> 
> This patch changes the type of firmware member of struct pmd_params to
> character array, to make memory resource release unnecessary, and
> changes the type of name member to character array, to keep the
> consistency of character string handling in struct pmd_params.
> 
> Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---

Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v4] net/softnic: fix memory leak in parsing arguments
  2021-07-16 12:04       ` Singh, Jasvinder
@ 2021-07-23  8:11         ` Thomas Monjalon
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2021-07-23  8:11 UTC (permalink / raw)
  To: Yu, DapengX; +Cc: Dumitrescu, Cristian, dev, stable, Singh, Jasvinder

> > From: Dapeng Yu <dapengx.yu@intel.com>
> > 
> > In function pmd_parse_args(), firmware path is duplicated from device
> > arguments as character string, but is never freed, which cause memory leak.
> > 
> > This patch changes the type of firmware member of struct pmd_params to
> > character array, to make memory resource release unnecessary, and
> > changes the type of name member to character array, to keep the
> > consistency of character string handling in struct pmd_params.
> > 
> > Fixes: 7e68bc20f8c8 ("net/softnic: restructure")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> 
> Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>

Applied, thanks.



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

end of thread, other threads:[~2021-07-23  8:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08  8:44 [dpdk-stable] [PATCH] net/softnic: fix memory leak in parsing arguments dapengx.yu
2021-07-13  8:44 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
2021-07-13 10:08 ` [dpdk-stable] [PATCH v2] " dapengx.yu
2021-07-13 10:10   ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
2021-07-14  5:47   ` [dpdk-stable] [PATCH v3] " dapengx.yu
2021-07-14 11:07     ` Singh, Jasvinder
2021-07-15  1:42       ` Yu, DapengX
2021-07-15  5:38     ` [dpdk-stable] [PATCH v4] " dapengx.yu
2021-07-16 12:04       ` Singh, Jasvinder
2021-07-23  8:11         ` [dpdk-stable] [dpdk-dev] " 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).