From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id BD58D44C3; Tue, 12 Mar 2019 15:44:58 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2456FC04BE07; Tue, 12 Mar 2019 14:44:58 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 710A44147; Tue, 12 Mar 2019 14:44:57 +0000 (UTC) From: Aaron Conole To: "Parthasarathy\, JananeeX M" Cc: "'dev\@dpdk.org'" , "Pattan\, Reshma" , "Rao\, Nikhil" , "'stable\@dpdk.org'" , "Poornima\, PallantlaX" References: <1549449822-412-1-git-send-email-pallantlax.poornima@intel.com> <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com> <7AE31235A30B41498D1C31348DC858BD5B54DCD4@IRSMSX103.ger.corp.intel.com> Date: Tue, 12 Mar 2019 10:44:56 -0400 In-Reply-To: <7AE31235A30B41498D1C31348DC858BD5B54DCD4@IRSMSX103.ger.corp.intel.com> (JananeeX M. Parthasarathy's message of "Tue, 12 Mar 2019 07:41:17 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 12 Mar 2019 14:44:58 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Mar 2019 14:44:59 -0000 "Parthasarathy, JananeeX M" writes: > Hi > >>-----Original Message----- >>From: Parthasarathy, JananeeX M >>Sent: Tuesday, February 19, 2019 6:33 PM >>To: Aaron Conole ; Poornima, PallantlaX >> >>Cc: dev@dpdk.org; Pattan, Reshma ; Rao, Nikhil >>; stable@dpdk.org >>Subject: RE: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf >> >> >> >>>-----Original Message----- >>>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Aaron Conole >>>Sent: Saturday, February 09, 2019 2:50 AM >>>To: Poornima, PallantlaX >>>Cc: dev@dpdk.org; Pattan, Reshma ; Rao, Nikhil >>>; stable@dpdk.org >>>Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with >>>snprintf >>> >>>Pallantla Poornima writes: >>> >>>> sprintf function is not secure as it doesn't check the length of string. >>>> More secure function snprintf is used. >>>> >>>> Fixes: 2a9c83ae3b ("test/eventdev: add multi-ports test") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Pallantla Poornima >>>> --- >>>> test/test/test_event_eth_rx_adapter.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/test/test/test_event_eth_rx_adapter.c >>>> b/test/test/test_event_eth_rx_adapter.c >>>> index 1d3be82b5..38f5c039f 100644 >>>> --- a/test/test/test_event_eth_rx_adapter.c >>>> +++ b/test/test/test_event_eth_rx_adapter.c >>>> @@ -479,7 +479,8 @@ adapter_multi_eth_add_del(void) >>>> /* add the max port for rx_adapter */ >>>> port_index = rte_eth_dev_count_total(); >>>> for (; port_index < RTE_MAX_ETHPORTS; port_index += 1) { >>>> - sprintf(driver_name, "%s%u", "net_null", drv_id); >>>> + snprintf(driver_name, sizeof(driver_name), "%s%u", "net_null", >>>> + drv_id); >>>> err = rte_vdev_init(driver_name, NULL); >>>> TEST_ASSERT(err == 0, "Failed driver %s got %d", >>>> driver_name, err); >>> >>>You call this a fix, but it's not possible for the value of drv_id to >>>exceed '32' and the buffer size is plenty accommodating for that. Did >>>I miss something? What is this fixing? >> >>It is better practice to use snprintf although in this case buffer will not overflow >>as size is big enough to accommodate. The changes were done mainly to >>replace sprintf to snprintf. Probably we can remove "fix" line as it is not issue in >>this scenario. >> >>Thanks >>M.P.Jananee > > Please suggest if we can remove "fix" line. This is a stylistic change, I don't think it's appropriate to call it a fix, so I think you can remove the "Fixes" line. On further reflection, I actually think it will still be wrong. If the size buffer is ever changed, what will happen on truncation? We don't get an overflow any longer, but we still pass an invalid argument, so I don't think this 'fix' is really even a fix. It still has a bug - albeit not one that immediately triggers SSP exception or stack overflow. Makes sense? > Thanks