[dpdk-dev] [PATCH] app/testpmd: fix copying the name of the dynflag

[dpdk-dev] [PATCH] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

When working with testpmd and setting the dynflag name, we copy the
name given by the cmd to the dynflag name.

The issue is that the size of the dynflag name is smaller then the
string used by testpmd.

This commit solves this issue by usign strncpy.

Coverity issue: 353610

Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")

Signed-off-by: Ori Kam <orika@mellanox.com>
---
 app/test-pmd/cmdline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index dab22bc..09429f9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18867,7 +18867,7 @@ struct cmd_config_tx_dynf_specific_result {
 		return;
 	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
 	if (flag <= 0) {
-		strcpy(desc_flag.name, res->name);
+		strncpy(desc_flag.name, res->name, sizeof(*desc_flag.name));
 		desc_flag.flags = 0;
 		flag = rte_mbuf_dynflag_register(&desc_flag);
 		if (flag < 0) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] app/testpmd: fix copying the name of the dynflag

[Iremonger, Bernard]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Thursday, January 30, 2020 1:23 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; orika@mellanox.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; viacheslavo@mellanox.com
> Subject: [PATCH] app/testpmd: fix copying the name of the dynflag
> 
> When working with testpmd and setting the dynflag name, we copy the
> name given by the cmd to the dynflag name.
> 
> The issue is that the size of the dynflag name is smaller then the string used
> by testpmd.
> 
> This commit solves this issue by usign strncpy.
> 
> Coverity issue: 353610
> 
> Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> dab22bc..09429f9 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -18867,7 +18867,7 @@ struct cmd_config_tx_dynf_specific_result {
>  		return;
>  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
>  	if (flag <= 0) {
> -		strcpy(desc_flag.name, res->name);
> +		strncpy(desc_flag.name, res->name,
> sizeof(*desc_flag.name));

This does not look correct to me.
Should it be sizeof(desc_flag.name[RTE_MBUF_DYN_NAMESIZE]));

>  		desc_flag.flags = 0;
>  		flag = rte_mbuf_dynflag_register(&desc_flag);
>  		if (flag < 0) {
> --
> 1.8.3.1

Regards,

Bernard.

Re: [dpdk-dev] [PATCH] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

Hi Bernard,

Thanks for review, PSB

Ori

> -----Original Message-----
> From: Iremonger, Bernard <bernard.iremonger@intel.com>
> Sent: Thursday, January 30, 2020 6:54 PM
> To: Ori Kam <orika@mellanox.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Subject: RE: [PATCH] app/testpmd: fix copying the name of the dynflag
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Thursday, January 30, 2020 1:23 PM
> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>
> > Cc: dev@dpdk.org; orika@mellanox.com; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; viacheslavo@mellanox.com
> > Subject: [PATCH] app/testpmd: fix copying the name of the dynflag
> >
> > When working with testpmd and setting the dynflag name, we copy the
> > name given by the cmd to the dynflag name.
> >
> > The issue is that the size of the dynflag name is smaller then the string used
> > by testpmd.
> >
> > This commit solves this issue by usign strncpy.
> >
> > Coverity issue: 353610
> >
> > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> >  app/test-pmd/cmdline.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > dab22bc..09429f9 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -18867,7 +18867,7 @@ struct cmd_config_tx_dynf_specific_result {
> >  		return;
> >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> >  	if (flag <= 0) {
> > -		strcpy(desc_flag.name, res->name);
> > +		strncpy(desc_flag.name, res->name,
> > sizeof(*desc_flag.name));
> 
> This does not look correct to me.
> Should it be sizeof(desc_flag.name[RTE_MBUF_DYN_NAMESIZE]));
>

You are correct, I will send a new version.
 
> >  		desc_flag.flags = 0;
> >  		flag = rte_mbuf_dynflag_register(&desc_flag);
> >  		if (flag < 0) {
> > --
> > 1.8.3.1
> 
> Regards,
> 
> Bernard.

Re: [dpdk-dev] [PATCH] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

Hi Bernard,

> -----Original Message-----
> From: Ori Kam
> Sent: Thursday, January 30, 2020 8:56 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Subject: RE: [PATCH] app/testpmd: fix copying the name of the dynflag
> 
> Hi Bernard,
> 
> Thanks for review, PSB
> 
> Ori
> 
> > -----Original Message-----
> > From: Iremonger, Bernard <bernard.iremonger@intel.com>
> > Sent: Thursday, January 30, 2020 6:54 PM
> > To: Ori Kam <orika@mellanox.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Slava Ovsiienko
> > <viacheslavo@mellanox.com>
> > Subject: RE: [PATCH] app/testpmd: fix copying the name of the dynflag
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Thursday, January 30, 2020 1:23 PM
> > > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Iremonger, Bernard
> > > <bernard.iremonger@intel.com>
> > > Cc: dev@dpdk.org; orika@mellanox.com; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>; viacheslavo@mellanox.com
> > > Subject: [PATCH] app/testpmd: fix copying the name of the dynflag
> > >
> > > When working with testpmd and setting the dynflag name, we copy the
> > > name given by the cmd to the dynflag name.
> > >
> > > The issue is that the size of the dynflag name is smaller then the string used
> > > by testpmd.
> > >
> > > This commit solves this issue by usign strncpy.
> > >
> > > Coverity issue: 353610
> > >
> > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> > >
> > > Signed-off-by: Ori Kam <orika@mellanox.com>
> > > ---
> > >  app/test-pmd/cmdline.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > dab22bc..09429f9 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -18867,7 +18867,7 @@ struct cmd_config_tx_dynf_specific_result {
> > >  		return;
> > >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> > >  	if (flag <= 0) {
> > > -		strcpy(desc_flag.name, res->name);
> > > +		strncpy(desc_flag.name, res->name,
> > > sizeof(*desc_flag.name));
> >
> > This does not look correct to me.
> > Should it be sizeof(desc_flag.name[RTE_MBUF_DYN_NAMESIZE]));
> >
> 
> You are correct, I will send a new version.
> 
I wasn't  clear I will fix the code but the fix is not like you suggested.
Your fix also doesn't work the return size will be 1.
The solution is sizeof(desc_flag.name)

> > >  		desc_flag.flags = 0;
> > >  		flag = rte_mbuf_dynflag_register(&desc_flag);
> > >  		if (flag < 0) {
> > > --
> > > 1.8.3.1
> >
> > Regards,
> >
> > Bernard.

[dpdk-dev] [PATCH v2] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

When working with testpmd and setting the dynflag name, we copy the
name given by the cmd to the dynflag name.

The issue is that the size of the dynflag name is smaller then the
string used by testpmd.

This commit solves this issue by checking if the requested name
is too long.

Coverity issue: 353610

Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")

Signed-off-by: Ori Kam <orika@mellanox.com>
---
V2:
 * Move len check to start of function.
---
 app/test-pmd/cmdline.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index dab22bc..de5ef41 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
 		return;
+	if (strlen(res->name) > sizeof(desc_flag.name)) {
+		printf("Flag name too long\n");
+		return;
+	}	
 	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
 	if (flag <= 0) {
 		strcpy(desc_flag.name, res->name);
-- 
1.8.3.1

[dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

When working with testpmd and setting the dynflag name, we copy the
name given by the cmd to the dynflag name.

The issue is that the size of the dynflag name is smaller then the
string used by testpmd.

This commit solves this issue by checking that the length of the requested
flag name is not too long.

Coverity issue: 353610

Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")

Signed-off-by: Ori Kam <orika@mellanox.com>
---
V3:
 * Fix style issue.

V2:
 * change to check the requested flag name.
---
 app/test-pmd/cmdline.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index dab22bc..7ccc778 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
 		return;
+	if (strlen(res->name) > sizeof(desc_flag.name)) {
+		printf("Flag name too long\n");
+		return;
+	}
 	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
 	if (flag <= 0) {
 		strcpy(desc_flag.name, res->name);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Ferruh Yigit]

On 1/30/2020 9:04 PM, Ori Kam wrote:
> When working with testpmd and setting the dynflag name, we copy the
> name given by the cmd to the dynflag name.
> 
> The issue is that the size of the dynflag name is smaller then the
> string used by testpmd.
> 
> This commit solves this issue by checking that the length of the requested
> flag name is not too long.
> 
> Coverity issue: 353610
> 
> Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
> V3:
>  * Fix style issue.
> 
> V2:
>  * change to check the requested flag name.
> ---
>  app/test-pmd/cmdline.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index dab22bc..7ccc778 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
>  
>  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>  		return;
> +	if (strlen(res->name) > sizeof(desc_flag.name)) {

Shouldn't it be ">=" since 'strlen' doesn't count terminating char.
It would be safer to use 'strnlen'.

> +		printf("Flag name too long\n");
> +		return;
> +	}
>  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
>  	if (flag <= 0) {
>  		strcpy(desc_flag.name, res->name);

And it would be nice to use 'strlcpy' here, to be sure target string will be
null terminated.

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

Hi Ferruh,
Thanks for your comments.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, January 31, 2020 4:02 PM
> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>
> Subject: Re: [PATCH v3] app/testpmd: fix copying the name of the dynflag
> 
> On 1/30/2020 9:04 PM, Ori Kam wrote:
> > When working with testpmd and setting the dynflag name, we copy the
> > name given by the cmd to the dynflag name.
> >
> > The issue is that the size of the dynflag name is smaller then the
> > string used by testpmd.
> >
> > This commit solves this issue by checking that the length of the requested
> > flag name is not too long.
> >
> > Coverity issue: 353610
> >
> > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> > V3:
> >  * Fix style issue.
> >
> > V2:
> >  * change to check the requested flag name.
> > ---
> >  app/test-pmd/cmdline.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > index dab22bc..7ccc778 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
> >
> >  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> >  		return;
> > +	if (strlen(res->name) > sizeof(desc_flag.name)) {
> 
> Shouldn't it be ">=" since 'strlen' doesn't count terminating char.
> It would be safer to use 'strnlen'.
>

 
Will fix.

> > +		printf("Flag name too long\n");
> > +		return;
> > +	}
> >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> >  	if (flag <= 0) {
> >  		strcpy(desc_flag.name, res->name);
> 
> And it would be nice to use 'strlcpy' here, to be sure target string will be
> null terminated.

Will fix.

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Iremonger, Bernard]

Hi Ori,

<snip>

> > Subject: Re: [PATCH v3] app/testpmd: fix copying the name of the
> > dynflag
> >
> > On 1/30/2020 9:04 PM, Ori Kam wrote:
> > > When working with testpmd and setting the dynflag name, we copy the
> > > name given by the cmd to the dynflag name.
> > >
> > > The issue is that the size of the dynflag name is smaller then the
> > > string used by testpmd.
> > >
> > > This commit solves this issue by checking that the length of the
> > > requested flag name is not too long.
> > >
> > > Coverity issue: 353610
> > >
> > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> > >
> > > Signed-off-by: Ori Kam <orika@mellanox.com>
> > > ---
> > > V3:
> > >  * Fix style issue.
> > >
> > > V2:
> > >  * change to check the requested flag name.
> > > ---
> > >  app/test-pmd/cmdline.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > dab22bc..7ccc778 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
> > >
> > >  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> > >  		return;
> > > +	if (strlen(res->name) > sizeof(desc_flag.name)) {

Would it be simpler to use RTE_MBUF_DYN_NAMESIZE instead of sizeof(desc_flag.name) ?

> >
> > Shouldn't it be ">=" since 'strlen' doesn't count terminating char.
> > It would be safer to use 'strnlen'.
> >
> 
> 
> Will fix.
> 
> > > +		printf("Flag name too long\n");
> > > +		return;
> > > +	}
> > >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> > >  	if (flag <= 0) {
> > >  		strcpy(desc_flag.name, res->name);
> >
> > And it would be nice to use 'strlcpy' here, to be sure target string
> > will be null terminated.
> 
> Will fix.

Regards,

Bernard.

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

Hi Bernard,


> -----Original Message-----
> From: Iremonger, Bernard <bernard.iremonger@intel.com>
> Sent: Tuesday, February 4, 2020 12:46 PM
> To: Ori Kam <orika@mellanox.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>
> Subject: RE: [PATCH v3] app/testpmd: fix copying the name of the dynflag
> 
> Hi Ori,
> 
> <snip>
> 
> > > Subject: Re: [PATCH v3] app/testpmd: fix copying the name of the
> > > dynflag
> > >
> > > On 1/30/2020 9:04 PM, Ori Kam wrote:
> > > > When working with testpmd and setting the dynflag name, we copy the
> > > > name given by the cmd to the dynflag name.
> > > >
> > > > The issue is that the size of the dynflag name is smaller then the
> > > > string used by testpmd.
> > > >
> > > > This commit solves this issue by checking that the length of the
> > > > requested flag name is not too long.
> > > >
> > > > Coverity issue: 353610
> > > >
> > > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> > > >
> > > > Signed-off-by: Ori Kam <orika@mellanox.com>
> > > > ---
> > > > V3:
> > > >  * Fix style issue.
> > > >
> > > > V2:
> > > >  * change to check the requested flag name.
> > > > ---
> > > >  app/test-pmd/cmdline.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > dab22bc..7ccc778 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -18865,6 +18865,10 @@ struct cmd_config_tx_dynf_specific_result {
> > > >
> > > >  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> > > >  		return;
> > > > +	if (strlen(res->name) > sizeof(desc_flag.name)) {
> 
> Would it be simpler to use RTE_MBUF_DYN_NAMESIZE instead of
> sizeof(desc_flag.name) ?
> 
I don't really care. I used the sizeof approach since it is more used in DPDK.
But If you wish I can move to define.

> > >
> > > Shouldn't it be ">=" since 'strlen' doesn't count terminating char.
> > > It would be safer to use 'strnlen'.
> > >
> >
> >
> > Will fix.
> >
> > > > +		printf("Flag name too long\n");
> > > > +		return;
> > > > +	}
> > > >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> > > >  	if (flag <= 0) {
> > > >  		strcpy(desc_flag.name, res->name);
> > >
> > > And it would be nice to use 'strlcpy' here, to be sure target string
> > > will be null terminated.
> >
> > Will fix.
> 
> Regards,
> 
> Bernard.

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Iremonger, Bernard]

Hi Ori,

<snip>


> > > > Subject: Re: [PATCH v3] app/testpmd: fix copying the name of the
> > > > dynflag
> > > >
> > > > On 1/30/2020 9:04 PM, Ori Kam wrote:
> > > > > When working with testpmd and setting the dynflag name, we copy
> > > > > the name given by the cmd to the dynflag name.
> > > > >
> > > > > The issue is that the size of the dynflag name is smaller then
> > > > > the string used by testpmd.
> > > > >
> > > > > This commit solves this issue by checking that the length of the
> > > > > requested flag name is not too long.
> > > > >
> > > > > Coverity issue: 353610
> > > > >
> > > > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> > > > >
> > > > > Signed-off-by: Ori Kam <orika@mellanox.com>
> > > > > ---
> > > > > V3:
> > > > >  * Fix style issue.
> > > > >
> > > > > V2:
> > > > >  * change to check the requested flag name.
> > > > > ---
> > > > >  app/test-pmd/cmdline.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > > > > index
> > > > > dab22bc..7ccc778 100644
> > > > > --- a/app/test-pmd/cmdline.c
> > > > > +++ b/app/test-pmd/cmdline.c
> > > > > @@ -18865,6 +18865,10 @@ struct
> > > > > cmd_config_tx_dynf_specific_result {
> > > > >
> > > > >  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> > > > >  		return;
> > > > > +	if (strlen(res->name) > sizeof(desc_flag.name)) {
> >
> > Would it be simpler to use RTE_MBUF_DYN_NAMESIZE instead of
> > sizeof(desc_flag.name) ?
> >
> I don't really care. I used the sizeof approach since it is more used in DPDK.
> But If you wish I can move to define.

For me it is simpler and clearer.

> 
> > > >
> > > > Shouldn't it be ">=" since 'strlen' doesn't count terminating char.

Not sure about ">="  as "=" does not allow space for terminating null.

> > > > It would be safer to use 'strnlen'.
> > > >
> > >
> > >
> > > Will fix.
> > >
> > > > > +		printf("Flag name too long\n");
> > > > > +		return;
> > > > > +	}
> > > > >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> > > > >  	if (flag <= 0) {
> > > > >  		strcpy(desc_flag.name, res->name);
> > > >
> > > > And it would be nice to use 'strlcpy' here, to be sure target
> > > > string will be null terminated.
> > >
> > > Will fix.
> >
Regards,

Bernard.

Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Iremonger, Bernard
> Sent: Tuesday, February 4, 2020 2:18 PM
> To: Ori Kam <orika@mellanox.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v3] app/testpmd: fix copying the name of the
> dynflag
> 
> Hi Ori,
> 
> <snip>
> 
> 
> > > > > Subject: Re: [PATCH v3] app/testpmd: fix copying the name of the
> > > > > dynflag
> > > > >
> > > > > On 1/30/2020 9:04 PM, Ori Kam wrote:
> > > > > > When working with testpmd and setting the dynflag name, we copy
> > > > > > the name given by the cmd to the dynflag name.
> > > > > >
> > > > > > The issue is that the size of the dynflag name is smaller then
> > > > > > the string used by testpmd.
> > > > > >
> > > > > > This commit solves this issue by checking that the length of the
> > > > > > requested flag name is not too long.
> > > > > >
> > > > > > Coverity issue: 353610
> > > > > >
> > > > > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> > > > > >
> > > > > > Signed-off-by: Ori Kam <orika@mellanox.com>
> > > > > > ---
> > > > > > V3:
> > > > > >  * Fix style issue.
> > > > > >
> > > > > > V2:
> > > > > >  * change to check the requested flag name.
> > > > > > ---
> > > > > >  app/test-pmd/cmdline.c | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > > > > > index
> > > > > > dab22bc..7ccc778 100644
> > > > > > --- a/app/test-pmd/cmdline.c
> > > > > > +++ b/app/test-pmd/cmdline.c
> > > > > > @@ -18865,6 +18865,10 @@ struct
> > > > > > cmd_config_tx_dynf_specific_result {
> > > > > >
> > > > > >  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> > > > > >  		return;
> > > > > > +	if (strlen(res->name) > sizeof(desc_flag.name)) {
> > >
> > > Would it be simpler to use RTE_MBUF_DYN_NAMESIZE instead of
> > > sizeof(desc_flag.name) ?
> > >
> > I don't really care. I used the sizeof approach since it is more used in DPDK.
> > But If you wish I can move to define.
> 
> For me it is simpler and clearer.

O.K will change.

> 
> >
> > > > >
> > > > > Shouldn't it be ">=" since 'strlen' doesn't count terminating char.
> 
> Not sure about ">="  as "=" does not allow space for terminating null.
> 

In new version I changed it to be >= means error.

> > > > > It would be safer to use 'strnlen'.
> > > > >
> > > >
> > > >
> > > > Will fix.
> > > >
> > > > > > +		printf("Flag name too long\n");
> > > > > > +		return;
> > > > > > +	}
> > > > > >  	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> > > > > >  	if (flag <= 0) {
> > > > > >  		strcpy(desc_flag.name, res->name);
> > > > >
> > > > > And it would be nice to use 'strlcpy' here, to be sure target
> > > > > string will be null terminated.
> > > >
> > > > Will fix.
> > >
> Regards,
> 
> Bernard.

[dpdk-dev] [PATCH v4] app/testpmd: fix copying the name of the dynflag

[Ori Kam]

When working with testpmd and setting the dynflag name, we copy the
name given by the cmd to the dynflag name.

The issue is that the size of the dynflag name is smaller then the
string used by testpmd.

This commit solves this issue by checking that the length of the requested
flag name is not too long.

Coverity issue: 353610

Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")

Signed-off-by: Ori Kam <orika@mellanox.com>
---
V4:
 * Address ML comments
V3:
 * Fix style issue.
V2:
 * change to check the requested flag name.
---
 app/test-pmd/cmdline.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index dab22bc..45602cc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18867,14 +18867,18 @@ struct cmd_config_tx_dynf_specific_result {
 		return;
 	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
 	if (flag <= 0) {
-		strcpy(desc_flag.name, res->name);
+		if (strlcpy(desc_flag.name, res->name,
+			    RTE_MBUF_DYN_NAMESIZE) >= RTE_MBUF_DYN_NAMESIZE) {
+			printf("Flag name too long\n");
+			return;
+		}
 		desc_flag.flags = 0;
 		flag = rte_mbuf_dynflag_register(&desc_flag);
 		if (flag < 0) {
 			printf("Can't register flag\n");
 			return;
 		}
-		strcpy(dynf_names[flag], res->name);
+		strcpy(dynf_names[flag], desc_flag.name);
 	}
 	old_port_flags = ports[res->port_id].mbuf_dynf;
 	if (!strcmp(res->value, "set")) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4] app/testpmd: fix copying the name of the dynflag

[Iremonger, Bernard]

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Tuesday, February 4, 2020 1:40 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; orika@mellanox.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; viacheslavo@mellanox.com
> Subject: [PATCH v4] app/testpmd: fix copying the name of the dynflag
> 
> When working with testpmd and setting the dynflag name, we copy the
> name given by the cmd to the dynflag name.
> 
> The issue is that the size of the dynflag name is smaller then the string used
> by testpmd.
> 
> This commit solves this issue by checking that the length of the requested
> flag name is not too long.
> 
> Coverity issue: 353610
> 
> Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Re: [dpdk-dev] [PATCH v4] app/testpmd: fix copying the name of the dynflag

[Ferruh Yigit]

On 2/4/2020 2:39 PM, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Ori Kam <orika@mellanox.com>
>> Sent: Tuesday, February 4, 2020 1:40 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>; Iremonger, Bernard
>> <bernard.iremonger@intel.com>
>> Cc: dev@dpdk.org; orika@mellanox.com; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; viacheslavo@mellanox.com
>> Subject: [PATCH v4] app/testpmd: fix copying the name of the dynflag
>>
>> When working with testpmd and setting the dynflag name, we copy the
>> name given by the cmd to the dynflag name.
>>
>> The issue is that the size of the dynflag name is smaller then the string used
>> by testpmd.
>>
>> This commit solves this issue by checking that the length of the requested
>> flag name is not too long.
>>
>> Coverity issue: 353610
>>
>> Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
>>
>> Signed-off-by: Ori Kam <orika@mellanox.com>
> 
> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 

Applied to dpdk-next-net/master, thanks.