From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7256E1B617 for ; Mon, 23 Oct 2017 15:30:20 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2017 06:30:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,423,1503385200"; d="scan'208";a="141244289" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.32]) by orsmga004.jf.intel.com with SMTP; 23 Oct 2017 06:30:15 -0700 Received: by (sSMTP sendmail emulation); Mon, 23 Oct 2017 14:30:15 +0100 Date: Mon, 23 Oct 2017 14:30:14 +0100 From: Bruce Richardson To: "Rybalchenko, Kirill" Cc: "dev@dpdk.org" , "Chilikin, Andrey" , "Xing, Beilei" , "Wu, Jingjing" Message-ID: <20171023133014.GA28528@bricha3-MOBL3.ger.corp.intel.com> References: <1508757889-40845-1-git-send-email-kirill.rybalchenko@intel.com> <20171023125745.GA26976@bricha3-MOBL3.ger.corp.intel.com> <696B43C21188DF4F9C9091AAE4789B824E2A50BE@IRSMSX108.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <696B43C21188DF4F9C9091AAE4789B824E2A50BE@IRSMSX108.ger.corp.intel.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.1 (2017-09-22) Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function 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: Mon, 23 Oct 2017 13:30:21 -0000 On Mon, Oct 23, 2017 at 02:06:05PM +0100, Rybalchenko, Kirill wrote: > > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Monday 23 October 2017 13:58 > > To: Rybalchenko, Kirill > > Cc: dev@dpdk.org; Chilikin, Andrey ; Xing, Beilei > > ; Wu, Jingjing > > Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for > > strncpy function > > > > On Mon, Oct 23, 2017 at 12:24:49PM +0100, Kirill Rybalchenko wrote: > > > num parameter for strncpy() function should be smaller than actual > > > destination buffer size to allow null termination. > > > > > > Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info") > > > > > > Signed-off-by: Kirill Rybalchenko > > > --- > > > drivers/net/i40e/rte_pmd_i40e.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/i40e/rte_pmd_i40e.c > > > b/drivers/net/i40e/rte_pmd_i40e.c index 4881ea0..489f66b 100644 > > > --- a/drivers/net/i40e/rte_pmd_i40e.c > > > +++ b/drivers/net/i40e/rte_pmd_i40e.c > > > @@ -1928,7 +1928,7 @@ int rte_pmd_i40e_get_ddp_info(uint8_t > > *pkg_buff, uint32_t pkg_size, > > > for (i = j = 0; i < nb_rec; j++) { > > > pinfo[j].proto_id = tlv->data[0]; > > > strncpy(pinfo[j].name, (const char *)&tlv->data[1], > > > - I40E_DDP_NAME_SIZE); > > > + I40E_DDP_NAME_SIZE - 1); > > > i += tlv->len; > > > tlv = &tlv[tlv->len]; > > > } > > > -- > > > > This is not a proper fix, as it still won't null-terminate the result. > > Replace strncpy with snprintf is probably the best solution. > The source buffer is 12 bytes long and guaranteed contains null-terminated C-string, > Destination buffer is 32 bytes long. Strncpy documentation says: > " Copies the first num characters of source to destination. If the end of the source C string (which is signaled by a null-character) is found before num characters have been copied, destination is padded with zeros until a total of num characters have been written to it." > I belive, having that, we can be sure that we'll get proper null-terminated string within destination buffer. > Or do I miss something? Well, yes and no. Given what you say, the patch proposed is pointless, given that both 32 and 31 are greater than 12. So, either we accept the existing code as correct as-is, and that it will always null-terminate because the destination buffer is bigger, or, we decide that we want to guarantee that the buffer will be null-terminated irrespective of the source and destination buffer sizes, in which case you need to use snprintf, or manually zero the last byte of the buffer. In either case, the patch you propose doesn't help, as reducing the destination buffer length to strncpy *never* causes strncpy to null-terminate where it failed to do so before. My 2c, in the absense of a sane strcpy function like strlcpy, snprintf should always be used in place of strncpy. Strncpy is just too error prone. /Bruce