diff mbox

[dpdk-dev] net/tap: fix coverity warning on strncpy

Message ID 20170217144426.47823-1-keith.wiles@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

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

Commit Message

Wiles, Keith Feb. 17, 2017, 2:44 p.m. UTC 
  Calling strncpy with a maximum size argument of 16 bytes on destination
array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the
destination string unterminated.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Wiles, Keith Feb. 17, 2017, 2:48 p.m. UTC | #1 
> On Feb 17, 2017, at 8:44 AM, Keith Wiles <keith.wiles@intel.com> wrote:

> 

> Calling strncpy with a maximum size argument of 16 bytes on destination

> array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the

> destination string unterminated.

> 

> Signed-off-by: Keith Wiles <keith.wiles@intel.com>

> ---

> drivers/net/tap/rte_eth_tap.c | 2 +-

> 1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c

> index efc4426..f9938d7 100644

> --- a/drivers/net/tap/rte_eth_tap.c

> +++ b/drivers/net/tap/rte_eth_tap.c

> @@ -297,7 +297,7 @@ tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)

> 		return -1;

> 	}

> 	memset(&ifr, 0, sizeof(ifr));

> -	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);

> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);

> 	err = ioctl(s, SIOCGIFFLAGS, &ifr);

> 	if (err < 0) {

> 		RTE_LOG(WARNING, PMD, "Unable to get %s device flags: %s\n”,


NAK missed the spaces around ‘-‘ :-(

> -- 

> 2.8.0.GIT

> 


Regards,
Keith
Bruce Richardson Feb. 17, 2017, 3:02 p.m. UTC | #2 
On Fri, Feb 17, 2017 at 08:44:26AM -0600, Keith Wiles wrote:
> Calling strncpy with a maximum size argument of 16 bytes on destination
> array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the
> destination string unterminated.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index efc4426..f9938d7 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -297,7 +297,7 @@ tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
>  		return -1;
>  	}
>  	memset(&ifr, 0, sizeof(ifr));
> -	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);
This is why I always prefer to use snprintf for copying strings, you
can't avoid null terminating.

	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);

	/Bruce
Wiles, Keith Feb. 17, 2017, 3:05 p.m. UTC | #3 
> On Feb 17, 2017, at 9:02 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> 
> On Fri, Feb 17, 2017 at 08:44:26AM -0600, Keith Wiles wrote:
>> Calling strncpy with a maximum size argument of 16 bytes on destination
>> array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the
>> destination string unterminated.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index efc4426..f9938d7 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -297,7 +297,7 @@ tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
>> 		return -1;
>> 	}
>> 	memset(&ifr, 0, sizeof(ifr));
>> -	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
>> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);
> This is why I always prefer to use snprintf for copying strings, you
> can't avoid null terminating.

Normally I use snprintf to not sure why I reverted to strncpy. Maybe leftover from a previous driver I used as the template.

> 
> 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> 
> 	/Bruce

Regards,
Keith
Bruce Richardson Feb. 17, 2017, 3:13 p.m. UTC | #4 
On Fri, Feb 17, 2017 at 03:05:40PM +0000, Wiles, Keith wrote:
> 
> > On Feb 17, 2017, at 9:02 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> > 
> > On Fri, Feb 17, 2017 at 08:44:26AM -0600, Keith Wiles wrote:
> >> Calling strncpy with a maximum size argument of 16 bytes on destination
> >> array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the
> >> destination string unterminated.
> >> 
> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> >> ---
> >> drivers/net/tap/rte_eth_tap.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >> index efc4426..f9938d7 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -297,7 +297,7 @@ tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
> >> 		return -1;
> >> 	}
> >> 	memset(&ifr, 0, sizeof(ifr));
> >> -	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
> >> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);
> > This is why I always prefer to use snprintf for copying strings, you
> > can't avoid null terminating.
> 
> Normally I use snprintf to not sure why I reverted to strncpy. Maybe leftover from a previous driver I used as the template.
> 
Is there a case to be made that DPDK should provide a strlcpy function
in the linuxapp EAL? [Assuming we don't want a dependency on libbsd?]
I find strncpy a horribly-error prone function to use - worse than
strcpy, since it gives a false sense of safety.

/Bruce
Ferruh Yigit Feb. 17, 2017, 3:15 p.m. UTC | #5 
On 2/17/2017 3:05 PM, Wiles, Keith wrote:
> 
>> On Feb 17, 2017, at 9:02 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
>>
>> On Fri, Feb 17, 2017 at 08:44:26AM -0600, Keith Wiles wrote:
>>> Calling strncpy with a maximum size argument of 16 bytes on destination
>>> array "ifr.ifr_ifrn.ifrn_name" of size 16 bytes might leave the
>>> destination string unterminated.
>>>
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index efc4426..f9938d7 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -297,7 +297,7 @@ tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
>>> 		return -1;
>>> 	}
>>> 	memset(&ifr, 0, sizeof(ifr));
>>> -	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
>>> +	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);
>> This is why I always prefer to use snprintf for copying strings, you
>> can't avoid null terminating.
> 
> Normally I use snprintf to not sure why I reverted to strncpy. Maybe leftover from a previous driver I used as the template.

Since you are already updating that line, do you prefer to convert it to
snprintf instead of above modification?

> 
>>
>> 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
>>
>> 	/Bruce
> 
> Regards,
> Keith
>
diff mbox

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index efc4426..f9938d7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -297,7 +297,7 @@  tap_link_set_flags(struct pmd_internals *pmd, short flags, int add)
 		return -1;
 	}
 	memset(&ifr, 0, sizeof(ifr));
-	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ);
+	strncpy(ifr.ifr_name, pmd->name, IFNAMSIZ-1);
 	err = ioctl(s, SIOCGIFFLAGS, &ifr);
 	if (err < 0) {
 		RTE_LOG(WARNING, PMD, "Unable to get %s device flags: %s\n",