diff mbox

[dpdk-dev,3/3] app/pdump: fix string overflow

Message ID 1466522285-15023-4-git-send-email-reshma.pattan@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Pattan, Reshma June 21, 2016, 3:18 p.m. UTC 
  using source length in strncpy can cause destination
overflow if destination length is not big enough to
handle the source string. Changes are made to use destination
size instead of source length in strncpy.

Coverity issue 127351: string overflow

Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
 app/pdump/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit June 21, 2016, 5:21 p.m. UTC | #1 
On 6/21/2016 4:18 PM, Reshma Pattan wrote:
> using source length in strncpy can cause destination
> overflow if destination length is not big enough to
> handle the source string. Changes are made to use destination
> size instead of source length in strncpy.
> 
> Coverity issue 127351: string overflow
> 
> Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
>  app/pdump/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index f8923b9..af92ef3 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -217,12 +217,12 @@ parse_rxtxdev(const char *key, const char *value, void *extra_args)
>  	struct pdump_tuples *pt = extra_args;
>  
>  	if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> -		strncpy(pt->rx_dev, value, strlen(value));
> +		strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);

I guess size-1 is to give room for terminating null byte, but for this
case is it guarantied that pt->rx_dev last byte is NULL?
Anupam Kapoor June 22, 2016, 6:46 a.m. UTC | #2 
>       if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> -             strncpy(pt->rx_dev, value, strlen(value));
> +             strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);

I guess size-1 is to give room for terminating null byte, but for this
case is it guarantied that pt->rx_dev last byte is NULL?

why not just use a snprintf(...) here since it has better error behavior ?
although compared to str*cpy it might be a bit slow, but hopefully that
should be ok ?

--
thanks
anupam


On Tue, Jun 21, 2016 at 10:51 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 6/21/2016 4:18 PM, Reshma Pattan wrote:
> > using source length in strncpy can cause destination
> > overflow if destination length is not big enough to
> > handle the source string. Changes are made to use destination
> > size instead of source length in strncpy.
> >
> > Coverity issue 127351: string overflow
> >
> > Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")
> >
> > Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> > ---
> >  app/pdump/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/pdump/main.c b/app/pdump/main.c
> > index f8923b9..af92ef3 100644
> > --- a/app/pdump/main.c
> > +++ b/app/pdump/main.c
> > @@ -217,12 +217,12 @@ parse_rxtxdev(const char *key, const char *value,
> void *extra_args)
> >       struct pdump_tuples *pt = extra_args;
> >
> >       if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> > -             strncpy(pt->rx_dev, value, strlen(value));
> > +             strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);
>
> I guess size-1 is to give room for terminating null byte, but for this
> case is it guarantied that pt->rx_dev last byte is NULL?
>
>
Bruce Richardson June 22, 2016, 9:21 a.m. UTC | #3 
On Wed, Jun 22, 2016 at 12:16:27PM +0530, Anupam Kapoor wrote:
> >       if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> > -             strncpy(pt->rx_dev, value, strlen(value));
> > +             strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);
> 
> I guess size-1 is to give room for terminating null byte, but for this
> case is it guarantied that pt->rx_dev last byte is NULL?
> 
> why not just use a snprintf(...) here since it has better error behavior ?
> although compared to str*cpy it might be a bit slow, but hopefully that
> should be ok ?
> 

Definite +1. For safely copying strings I think snprintf is often the easiest
API to use.

/Bruce

> --
> thanks
> anupam
> 
> 
> On Tue, Jun 21, 2016 at 10:51 PM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> 
> > On 6/21/2016 4:18 PM, Reshma Pattan wrote:
> > > using source length in strncpy can cause destination
> > > overflow if destination length is not big enough to
> > > handle the source string. Changes are made to use destination
> > > size instead of source length in strncpy.
> > >
> > > Coverity issue 127351: string overflow
> > >
> > > Fixes: caa7028276b8 ("app/pdump: add tool for packet capturing")
> > >
> > > Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> > > ---
> > >  app/pdump/main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/pdump/main.c b/app/pdump/main.c
> > > index f8923b9..af92ef3 100644
> > > --- a/app/pdump/main.c
> > > +++ b/app/pdump/main.c
> > > @@ -217,12 +217,12 @@ parse_rxtxdev(const char *key, const char *value,
> > void *extra_args)
> > >       struct pdump_tuples *pt = extra_args;
> > >
> > >       if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> > > -             strncpy(pt->rx_dev, value, strlen(value));
> > > +             strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);
> >
> > I guess size-1 is to give room for terminating null byte, but for this
> > case is it guarantied that pt->rx_dev last byte is NULL?
> >
> >
> 
> 
> -- 
> In the beginning was the lambda, and the lambda was with Emacs, and Emacs
> was the lambda.
Pattan, Reshma June 22, 2016, 9:24 a.m. UTC | #4 
Hi,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, June 22, 2016 10:22 AM
> To: Anupam Kapoor <anupam.kapoor@gmail.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 3/3] app/pdump: fix string overflow
> 
> On Wed, Jun 22, 2016 at 12:16:27PM +0530, Anupam Kapoor wrote:
> > >       if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
> > > -             strncpy(pt->rx_dev, value, strlen(value));
> > > +             strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);
> >
> > I guess size-1 is to give room for terminating null byte, but for this
> > case is it guarantied that pt->rx_dev last byte is NULL?
> >
> > why not just use a snprintf(...) here since it has better error behavior ?
> > although compared to str*cpy it might be a bit slow, but hopefully
> > that should be ok ?
> >
> 
> Definite +1. For safely copying strings I think snprintf is often the easiest API to
> use.
> 

Ok, will make the changes.

Thanks,
Reshma
diff mbox

Patch

diff --git a/app/pdump/main.c b/app/pdump/main.c
index f8923b9..af92ef3 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -217,12 +217,12 @@  parse_rxtxdev(const char *key, const char *value, void *extra_args)
 	struct pdump_tuples *pt = extra_args;
 
 	if (!strcmp(key, PDUMP_RX_DEV_ARG)) {
-		strncpy(pt->rx_dev, value, strlen(value));
+		strncpy(pt->rx_dev, value, sizeof(pt->rx_dev)-1);
 		/* identify the tx stream type for pcap vdev */
 		if (if_nametoindex(pt->rx_dev))
 			pt->rx_vdev_stream_type = IFACE;
 	} else if (!strcmp(key, PDUMP_TX_DEV_ARG)) {
-		strncpy(pt->tx_dev, value, strlen(value));
+		strncpy(pt->tx_dev, value, sizeof(pt->tx_dev)-1);
 		/* identify the tx stream type for pcap vdev */
 		if (if_nametoindex(pt->tx_dev))
 			pt->tx_vdev_stream_type = IFACE;