diff mbox

[dpdk-dev] net/i40e: fix value of num parameter for strncpy function

Message ID 1508757889-40845-1-git-send-email-kirill.rybalchenko@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Rybalchenko, Kirill Oct. 23, 2017, 11:24 a.m. UTC 
  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 <kirill.rybalchenko@intel.com>
---
 drivers/net/i40e/rte_pmd_i40e.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bruce Richardson Oct. 23, 2017, 12:57 p.m. UTC | #1 
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 <kirill.rybalchenko@intel.com>
> ---
>  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.

/Bruce
Rybalchenko, Kirill Oct. 23, 2017, 1:06 p.m. UTC | #2 
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday 23 October 2017 13:58
> To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>
> Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> 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 <kirill.rybalchenko@intel.com>
> > ---
> >  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?
> 
> /Bruce
Bruce Richardson Oct. 23, 2017, 1:30 p.m. UTC | #3 
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 <kirill.rybalchenko@intel.com>
> > Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > 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 <kirill.rybalchenko@intel.com>
> > > ---
> > >  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
diff mbox

Patch

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];
 		}