diff mbox series

[v2] net/softnic: fix memory leak in parsing arguments

Message ID 20210713100801.597956-1-dapengx.yu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series [v2] net/softnic: fix memory leak in parsing arguments |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-abi-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-testing fail Testing issues
ci/iol-intel-Performance fail Performance Testing issues

Commit Message

Yu, DapengX July 13, 2021, 10:08 a.m. UTC 
  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(-)

Comments

Andrew Rybchenko July 13, 2021, 10:10 a.m. UTC | #1 
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. */
>
Andrew Rybchenko July 13, 2021, 10:29 a.m. UTC | #2 
Adding list and maintainers back.

On 7/13/21 1:18 PM, Yu, DapengX wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, July 13, 2021 6:11 PM
>> To: Yu, DapengX <dapengx.yu@intel.com>; Singh, Jasvinder
>> <jasvinder.singh@intel.com>; Dumitrescu, Cristian
>> <cristian.dumitrescu@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] net/softnic: fix memory leak in parsing
>> arguments
>>
>> 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.
> If the return value is not checked here, and the dst path string is not complete when the path string is too long, softnic_cli_script_process() will prompt error.
> So the fix will not cause more negative impact.
> And for the similar case below, the src and dst string length is same always,  so no negative impact too.

I see. For me it sounds a bit fragile, but not critical.
Anyway since I'm not 100% sure I'll wait for maintainers
review.

>>
>>>  	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. */
>>>
>
diff mbox series

Patch

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. */