From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D358EA0C4A; Tue, 13 Jul 2021 12:29:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C02BF41227; Tue, 13 Jul 2021 12:29:17 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 1B35540687 for ; Tue, 13 Jul 2021 12:29:16 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id A45F47F510; Tue, 13 Jul 2021 13:29:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A45F47F510 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626172155; bh=q2+FUKDqp5keLoqmNfuLkfffq0d6EJlLf6ES9MwlIYY=; h=Subject:To:References:From:Cc:Date:In-Reply-To; b=qixtuchRkPl5ulfsBuYUJKGTmUvyGqet/dRLWeZcQIkOti1lpRggMEObxwv2Xrs0T P3aGUX9HaNwb1j039RfzBZRqxFpBObZuhAKSftpzLigkeSIfB86cgwjXPdltX5hMCp UQmhTRF9aN5BR19e6AeKS8JvuF40Q/WH9htg0DvE= To: "Yu, DapengX" References: <20210708084457.690538-1-dapengx.yu@intel.com> <20210713100801.597956-1-dapengx.yu@intel.com> <85aa208a-6e06-a06c-408b-0982dd175ef9@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Cc: Cristian Dumitrescu , Jasvinder Singh , "dev@dpdk.org" Message-ID: Date: Tue, 13 Jul 2021 13:29:15 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] net/softnic: fix memory leak in parsing arguments X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Adding list and maintainers back. On 7/13/21 1:18 PM, Yu, DapengX wrote: >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Tuesday, July 13, 2021 6:11 PM >> To: Yu, DapengX ; Singh, Jasvinder >> ; Dumitrescu, Cristian >> >> 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 >>> >>> 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 >>> --- >>> 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. */ >>> >