Adding list and maintainers back. On 7/13/21 1:18 PM, Yu, DapengX wrote: >> -----Original Message----- >> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> >> Sent: Tuesday, July 13, 2021 6:11 PM >> To: Yu, DapengX <dapengx...@intel.com>; Singh, Jasvinder >> <jasvinder.si...@intel.com>; Dumitrescu, Cristian >> <cristian.dumitre...@intel.com> >> Cc: dev@dpdk.org; sta...@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v2] net/softnic: fix memory leak in parsing >> arguments >> >> On 7/13/21 1:08 PM, dapengx...@intel.com wrote: >>> From: Dapeng Yu <dapengx...@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: sta...@dpdk.org >>> >>> Signed-off-by: Dapeng Yu <dapengx...@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. */ >>> >