diff mbox

[dpdk-dev] lib/cmdline: init parse result memeory

Message ID 20171115155402.9967-1-xuemingl@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Xueming Li Nov. 15, 2017, 3:54 p.m. UTC 
  Initialize binary result memory before parsing to avoid garbage in
parsing result.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_cmdline/cmdline_parse.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Olivier Matz Dec. 7, 2017, 2:48 p.m. UTC | #1 
On Wed, Nov 15, 2017 at 11:54:02PM +0800, Xueming Li wrote:
> Initialize binary result memory before parsing to avoid garbage in
> parsing result.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> ---
>  lib/librte_cmdline/cmdline_parse.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
> index 3e12ee54f..9124758f1 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -267,6 +267,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  	if (!cl || !buf)
>  		return CMDLINE_PARSE_BAD_ARGS;
>  
> +	memset(tmp_result.buf, 0, sizeof(tmp_result.buf));
> +
>  	ctx = cl->ctx;
>  
>  	/*


Did you see an issue (a bug or a crash) without the memset()?
Or is it to avoid filling unused fields in the parsed struct?

I'm not sure if your patch is enough: cmdline_parse() calls match_inst()
for each registered command. If a command partially matches (only the
first tokens), the buffer is modified. So the next one will start
with a dirty buffer.

I suggest to put the memset() in match_inst() instead. Something
like this:

	if (resbuf != NULL)
		memset(resbuf, 0, resbuf_size);


It will reset the buffer before using it.

Olivier
Xueming Li Dec. 7, 2017, 3:05 p.m. UTC | #2 
Hi Olivier,

> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Thursday, December 7, 2017 10:48 PM

> To: Xueming(Steven) Li <xuemingl@mellanox.com>

> Cc: dev@dpdk.org

> Subject: Re: [PATCH] lib/cmdline: init parse result memeory

> 

> On Wed, Nov 15, 2017 at 11:54:02PM +0800, Xueming Li wrote:

> > Initialize binary result memory before parsing to avoid garbage in

> > parsing result.

> >

> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>

> > ---

> >  lib/librte_cmdline/cmdline_parse.c | 2 ++

> >  1 file changed, 2 insertions(+)

> >

> > diff --git a/lib/librte_cmdline/cmdline_parse.c

> > b/lib/librte_cmdline/cmdline_parse.c

> > index 3e12ee54f..9124758f1 100644

> > --- a/lib/librte_cmdline/cmdline_parse.c

> > +++ b/lib/librte_cmdline/cmdline_parse.c

> > @@ -267,6 +267,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)

> >  	if (!cl || !buf)

> >  		return CMDLINE_PARSE_BAD_ARGS;

> >

> > +	memset(tmp_result.buf, 0, sizeof(tmp_result.buf));

> > +

> >  	ctx = cl->ctx;

> >

> >  	/*

> 

> 

> Did you see an issue (a bug or a crash) without the memset()?

> Or is it to avoid filling unused fields in the parsed struct?

Yes, I'm using same struct for some similar commands, have to avoid filling 
unused fields.

> 

> I'm not sure if your patch is enough: cmdline_parse() calls match_inst()

> for each registered command. If a command partially matches (only the

> first tokens), the buffer is modified. So the next one will start with a

> dirty buffer.

> 

> I suggest to put the memset() in match_inst() instead. Something like this:

> 

> 	if (resbuf != NULL)

> 		memset(resbuf, 0, resbuf_size);

> 

> 

> It will reset the buffer before using it.

It was there for performance concern, since the buffer could be tainted, I'll
upload a new version according to your suggestion, appreciate your suggestion.

> 

> Olivier
Xueming Li Dec. 7, 2017, 3:35 p.m. UTC | #3 
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xueming(Steven) Li

> Sent: Thursday, December 7, 2017 11:05 PM

> To: Olivier MATZ <olivier.matz@6wind.com>

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH] lib/cmdline: init parse result memeory

> 

> Hi Olivier,

> 

> > -----Original Message-----

> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> > Sent: Thursday, December 7, 2017 10:48 PM

> > To: Xueming(Steven) Li <xuemingl@mellanox.com>

> > Cc: dev@dpdk.org

> > Subject: Re: [PATCH] lib/cmdline: init parse result memeory

> >

> > On Wed, Nov 15, 2017 at 11:54:02PM +0800, Xueming Li wrote:

> > > Initialize binary result memory before parsing to avoid garbage in

> > > parsing result.

> > >

> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>

> > > ---

> > >  lib/librte_cmdline/cmdline_parse.c | 2 ++

> > >  1 file changed, 2 insertions(+)

> > >

> > > diff --git a/lib/librte_cmdline/cmdline_parse.c

> > > b/lib/librte_cmdline/cmdline_parse.c

> > > index 3e12ee54f..9124758f1 100644

> > > --- a/lib/librte_cmdline/cmdline_parse.c

> > > +++ b/lib/librte_cmdline/cmdline_parse.c

> > > @@ -267,6 +267,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)

> > >  	if (!cl || !buf)

> > >  		return CMDLINE_PARSE_BAD_ARGS;

> > >

> > > +	memset(tmp_result.buf, 0, sizeof(tmp_result.buf));

> > > +

> > >  	ctx = cl->ctx;

> > >

> > >  	/*

> >

> >

> > Did you see an issue (a bug or a crash) without the memset()?

> > Or is it to avoid filling unused fields in the parsed struct?

> Yes, I'm using same struct for some similar commands, have to avoid

> filling unused fields.

> 

> >

> > I'm not sure if your patch is enough: cmdline_parse() calls

> > match_inst() for each registered command. If a command partially

> > matches (only the first tokens), the buffer is modified. So the next

> > one will start with a dirty buffer.

> >

> > I suggest to put the memset() in match_inst() instead. Something like

> this:

> >

> > 	if (resbuf != NULL)

> > 		memset(resbuf, 0, resbuf_size);

> >

> >

> > It will reset the buffer before using it.

> It was there for performance concern, since the buffer could be tainted,

> I'll upload a new version according to your suggestion, appreciate your

> suggestion.

Rte_flow CLI seems to be relying on modified buffer, add Adrien:
testpmd> flow create 0 ingress pattern eth  / ipv4 / udp / vxlan / end actions rss queues 1 2 end level 1 / mark id 0x123456  / end
Caught error type 2 (flow rule (handle)): no valid action

> 

> >

> > Olivier
Adrien Mazarguil Dec. 7, 2017, 5:13 p.m. UTC | #4 
On Thu, Dec 07, 2017 at 03:35:24PM +0000, Xueming(Steven) Li wrote:
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xueming(Steven) Li
> > Sent: Thursday, December 7, 2017 11:05 PM
> > To: Olivier MATZ <olivier.matz@6wind.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] lib/cmdline: init parse result memeory
> > 
> > Hi Olivier,
> > 
> > > -----Original Message-----
> > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > Sent: Thursday, December 7, 2017 10:48 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [PATCH] lib/cmdline: init parse result memeory
> > >
> > > On Wed, Nov 15, 2017 at 11:54:02PM +0800, Xueming Li wrote:
> > > > Initialize binary result memory before parsing to avoid garbage in
> > > > parsing result.
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > > ---
> > > >  lib/librte_cmdline/cmdline_parse.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/lib/librte_cmdline/cmdline_parse.c
> > > > b/lib/librte_cmdline/cmdline_parse.c
> > > > index 3e12ee54f..9124758f1 100644
> > > > --- a/lib/librte_cmdline/cmdline_parse.c
> > > > +++ b/lib/librte_cmdline/cmdline_parse.c
> > > > @@ -267,6 +267,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> > > >  	if (!cl || !buf)
> > > >  		return CMDLINE_PARSE_BAD_ARGS;
> > > >
> > > > +	memset(tmp_result.buf, 0, sizeof(tmp_result.buf));
> > > > +
> > > >  	ctx = cl->ctx;
> > > >
> > > >  	/*
> > >
> > >
> > > Did you see an issue (a bug or a crash) without the memset()?
> > > Or is it to avoid filling unused fields in the parsed struct?
> > Yes, I'm using same struct for some similar commands, have to avoid
> > filling unused fields.
> > 
> > >
> > > I'm not sure if your patch is enough: cmdline_parse() calls
> > > match_inst() for each registered command. If a command partially
> > > matches (only the first tokens), the buffer is modified. So the next
> > > one will start with a dirty buffer.
> > >
> > > I suggest to put the memset() in match_inst() instead. Something like
> > this:
> > >
> > > 	if (resbuf != NULL)
> > > 		memset(resbuf, 0, resbuf_size);
> > >
> > >
> > > It will reset the buffer before using it.
> > It was there for performance concern, since the buffer could be tainted,
> > I'll upload a new version according to your suggestion, appreciate your
> > suggestion.
> Rte_flow CLI seems to be relying on modified buffer, add Adrien:
> testpmd> flow create 0 ingress pattern eth  / ipv4 / udp / vxlan / end actions rss queues 1 2 end level 1 / mark id 0x123456  / end
> Caught error type 2 (flow rule (handle)): no valid action

While the flow command relies on the contents of this buffer when parsing
tokens, it doesn't expect it to be maintained across match_inst() calls.

In fact another issue prevents Olivier's suggestion from working.
Commit 9b3fbb051d2e ("cmdline: fix parsing") is supposed prevent further
match_inst() calls after the right inst is found but doesn't break
cmdline_parse()'s loop; subsequent iterations clear the result buffer.

Here's a suggestion to try:

 diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
 index 3e12ee5..e967410 100644
 --- a/lib/librte_cmdline/cmdline_parse.c
 +++ b/lib/librte_cmdline/cmdline_parse.c
 @@ -338,8 +338,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
                                         err = CMDLINE_PARSE_AMBIGUOUS;
                                         f=NULL;
                                         debug_printf("Ambiguous cmd\n");
 -                                       break;
                                 }
 +                               break;
                         }
                 }
diff mbox

Patch

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 3e12ee54f..9124758f1 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -267,6 +267,8 @@  cmdline_parse(struct cmdline *cl, const char * buf)
 	if (!cl || !buf)
 		return CMDLINE_PARSE_BAD_ARGS;
 
+	memset(tmp_result.buf, 0, sizeof(tmp_result.buf));
+
 	ctx = cl->ctx;
 
 	/*