diff mbox series

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

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

Checks

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

Commit Message

Yu, DapengX July 14, 2021, 5:47 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.
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(-)

Comments

Jasvinder Singh July 14, 2021, 11:07 a.m. UTC | #1
<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
Yu, DapengX July 15, 2021, 1:42 a.m. UTC | #2
> -----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
diff mbox series

Patch

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