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

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

[Xueming Li]

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;
 
 	/*
-- 
2.13.3

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

[Olivier MATZ]

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

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

[Xueming(Steven) Li]

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

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

[Xueming(Steven) Li]

> -----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

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

[Adrien Mazarguil]

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

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Xueming Li]

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 3e12ee54f..4072103f2 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -168,6 +168,9 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 	int n = 0;
 	struct cmdline_token_hdr token_hdr;
 
+	if (resbuf != NULL)
+		memset(resbuf, 0, resbuf_size);
+
 	/* check if we match all tokens of inst */
 	while (!nb_match_token || i < nb_match_token) {
 		token_p = get_token(inst, i);
@@ -338,8 +341,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 					err = CMDLINE_PARSE_AMBIGUOUS;
 					f=NULL;
 					debug_printf("Ambiguous cmd\n");
-					break;
 				}
+				break;
 			}
 		}
 
-- 
2.13.3

Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Adrien Mazarguil]

On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote:
> Initialize binary result memory before parsing to avoid garbage in
> parsing result.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

Since you chose to move the break statement, maybe the original commit
mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing") can
be reverted afterward? I think it makes tmp_result redundant.

Wenzhuo, as the author of that commit, can you confirm?

Olivier, no problem with breaking the loop immediately after the first
successful match_inst() call instead of the last one? (I don't see why it
would be an issue but I may have missed something)

> ---
>  lib/librte_cmdline/cmdline_parse.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
> index 3e12ee54f..4072103f2 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -168,6 +168,9 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
>  	int n = 0;
>  	struct cmdline_token_hdr token_hdr;
>  
> +	if (resbuf != NULL)
> +		memset(resbuf, 0, resbuf_size);
> +
>  	/* check if we match all tokens of inst */
>  	while (!nb_match_token || i < nb_match_token) {
>  		token_p = get_token(inst, i);
> @@ -338,8 +341,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  					err = CMDLINE_PARSE_AMBIGUOUS;
>  					f=NULL;
>  					debug_printf("Ambiguous cmd\n");
> -					break;
>  				}
> +				break;
>  			}
>  		}
>  
> -- 
> 2.13.3
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Olivier MATZ]

On Fri, Dec 08, 2017 at 01:27:26PM +0100, Adrien Mazarguil wrote:
> On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote:
> > Initialize binary result memory before parsing to avoid garbage in
> > parsing result.
> > 
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> Since you chose to move the break statement, maybe the original commit
> mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing") can
> be reverted afterward? I think it makes tmp_result redundant.
> 
> Wenzhuo, as the author of that commit, can you confirm?
> 
> Olivier, no problem with breaking the loop immediately after the first
> successful match_inst() call instead of the last one? (I don't see why it
> would be an issue but I may have missed something)

Moving the break will change the behavior, it will never detect
ambiguous commands (i.e when several commands match the same input).
So I think we should not do it.

IMO, only the memset() is enough.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Xueming(Steven) Li]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, December 8, 2017 9:51 PM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory
> 
> On Fri, Dec 08, 2017 at 01:27:26PM +0100, Adrien Mazarguil wrote:
> > On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote:
> > > Initialize binary result memory before parsing to avoid garbage in
> > > parsing result.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> >
> > Since you chose to move the break statement, maybe the original commit
> > mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing")
> > can be reverted afterward? I think it makes tmp_result redundant.
> >
> > Wenzhuo, as the author of that commit, can you confirm?
> >
> > Olivier, no problem with breaking the loop immediately after the first
> > successful match_inst() call instead of the last one? (I don't see why
> > it would be an issue but I may have missed something)
> 
> Moving the break will change the behavior, it will never detect ambiguous
> commands (i.e when several commands match the same input).
> So I think we should not do it.
> 
We don't have CLI performance issue all the time, but recently I'm using files 
to save hundreds of batch 'expect' test commands, execute with a 'load' command,
so performance of CLI processing becomes important.
CLI developer should make sure not defining a CLI that already exists and verify
the new CLI working as expected.

> IMO, only the memset() is enough.
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Adrien Mazarguil]

On Fri, Dec 08, 2017 at 02:51:15PM +0100, Olivier MATZ wrote:
> On Fri, Dec 08, 2017 at 01:27:26PM +0100, Adrien Mazarguil wrote:
> > On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote:
> > > Initialize binary result memory before parsing to avoid garbage in
> > > parsing result.
> > > 
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > 
> > Since you chose to move the break statement, maybe the original commit
> > mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing") can
> > be reverted afterward? I think it makes tmp_result redundant.
> > 
> > Wenzhuo, as the author of that commit, can you confirm?
> > 
> > Olivier, no problem with breaking the loop immediately after the first
> > successful match_inst() call instead of the last one? (I don't see why it
> > would be an issue but I may have missed something)
> 
> Moving the break will change the behavior, it will never detect
> ambiguous commands (i.e when several commands match the same input).
> So I think we should not do it.
> 
> IMO, only the memset() is enough.

I agree it should be, however as reported by Xueming doing so somehow breaks
the flow command. In my previous reply I assumed that was caused by clearing
the result buffer of prior successful calls in cmdline_parse(), I just
checked and it appears not to be the case. Wenzhuo's patch works fine.

The root cause is actually the flow command stores internal buffer addresses
in the output buffer, which happens to be tmp_result.buf since commit
9b3fbb051d2e.

When match_inst() returns successfully, tmp_result.buf is copied to
result.buf and the contents of tmp_result.buf are discarded by the next call
to match_inst(). Addresses stored inside result.buf still refer to locations
inside tmp_result.buf, memset()'ing that region only makes that bug manifest
itself.

Another suggestion to address the underlying issue before adding memset():

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 205f243..15a3482 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -254,7 +254,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 	union {
 		char buf[CMDLINE_PARSE_RESULT_BUFSIZE];
 		long double align; /* strong alignment constraint for buf */
-	} result, tmp_result;
+	} result, result_ok;
 	void (*f)(void *, struct cmdline *, void *) = NULL;
 	void *data = NULL;
 	int comment = 0;
@@ -315,16 +315,13 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 		debug_printf("INST %d\n", inst_num);
 
 		/* fully parsed */
-		tok = match_inst(inst, buf, 0, tmp_result.buf,
-				 sizeof(tmp_result.buf));
+		tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf));
 
 		if (tok > 0) /* we matched at least one token */
 			err = CMDLINE_PARSE_BAD_ARGS;
 
 		else if (!tok) {
 			debug_printf("INST fully parsed\n");
-			memcpy(&result, &tmp_result,
-			       sizeof(result));
 			/* skip spaces */
 			while (isblank2(*curbuf)) {
 				curbuf++;
@@ -344,6 +341,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 					break;
 				}
 			}
+			result_ok = result;
 		}
 
 		inst_num ++;
@@ -352,6 +350,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* call func */
 	if (f) {
+		result = result_ok;
 		f(result.buf, cl, data);
 	}

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory

[Olivier MATZ]

Hi,

On Fri, Dec 08, 2017 at 04:04:33PM +0100, Adrien Mazarguil wrote:
> On Fri, Dec 08, 2017 at 02:51:15PM +0100, Olivier MATZ wrote:
> > On Fri, Dec 08, 2017 at 01:27:26PM +0100, Adrien Mazarguil wrote:
> > > On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote:
> > > > Initialize binary result memory before parsing to avoid garbage in
> > > > parsing result.
> > > > 
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > 
> > > Since you chose to move the break statement, maybe the original commit
> > > mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing") can
> > > be reverted afterward? I think it makes tmp_result redundant.
> > > 
> > > Wenzhuo, as the author of that commit, can you confirm?
> > > 
> > > Olivier, no problem with breaking the loop immediately after the first
> > > successful match_inst() call instead of the last one? (I don't see why it
> > > would be an issue but I may have missed something)
> > 
> > Moving the break will change the behavior, it will never detect
> > ambiguous commands (i.e when several commands match the same input).
> > So I think we should not do it.
> > 
> > IMO, only the memset() is enough.
> 
> I agree it should be, however as reported by Xueming doing so somehow breaks
> the flow command. In my previous reply I assumed that was caused by clearing
> the result buffer of prior successful calls in cmdline_parse(), I just
> checked and it appears not to be the case. Wenzhuo's patch works fine.
> 
> The root cause is actually the flow command stores internal buffer addresses
> in the output buffer, which happens to be tmp_result.buf since commit
> 9b3fbb051d2e.
> 
> When match_inst() returns successfully, tmp_result.buf is copied to
> result.buf and the contents of tmp_result.buf are discarded by the next call
> to match_inst(). Addresses stored inside result.buf still refer to locations
> inside tmp_result.buf, memset()'ing that region only makes that bug manifest
> itself.
> 
> Another suggestion to address the underlying issue before adding memset():
> 
> diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
> index 205f243..15a3482 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -254,7 +254,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  	union {
>  		char buf[CMDLINE_PARSE_RESULT_BUFSIZE];
>  		long double align; /* strong alignment constraint for buf */
> -	} result, tmp_result;
> +	} result, result_ok;
>  	void (*f)(void *, struct cmdline *, void *) = NULL;
>  	void *data = NULL;
>  	int comment = 0;
> @@ -315,16 +315,13 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  		debug_printf("INST %d\n", inst_num);
>  
>  		/* fully parsed */
> -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> -				 sizeof(tmp_result.buf));
> +		tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf));
>  
>  		if (tok > 0) /* we matched at least one token */
>  			err = CMDLINE_PARSE_BAD_ARGS;
>  
>  		else if (!tok) {
>  			debug_printf("INST fully parsed\n");
> -			memcpy(&result, &tmp_result,
> -			       sizeof(result));
>  			/* skip spaces */
>  			while (isblank2(*curbuf)) {
>  				curbuf++;
> @@ -344,6 +341,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  					break;
>  				}
>  			}
> +			result_ok = result;
>  		}
>  
>  		inst_num ++;
> @@ -352,6 +350,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  
>  	/* call func */
>  	if (f) {
> +		result = result_ok;
>  		f(result.buf, cl, data);
>  	}
> 

In addition to the memset() at the beginning of match_inst(), it looks
good to me.

A particular attention should be paid to the explanation of the issue
and its solution in the commit log :)

[dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Xueming Li]

Initialize result memory every time before parsing. Also save
successfully parsed result before further ambiguous command detection to
avoid result being tainted by later parsing.

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

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 3e12ee54f..45117789a 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -168,6 +168,9 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 	int n = 0;
 	struct cmdline_token_hdr token_hdr;
 
+	if (resbuf != NULL)
+		memset(resbuf, 0, resbuf_size);
+
 	/* check if we match all tokens of inst */
 	while (!nb_match_token || i < nb_match_token) {
 		token_p = get_token(inst, i);
@@ -251,7 +254,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 	union {
 		char buf[CMDLINE_PARSE_RESULT_BUFSIZE];
 		long double align; /* strong alignment constraint for buf */
-	} result, tmp_result;
+	} result, result_ok;
 	void (*f)(void *, struct cmdline *, void *) = NULL;
 	void *data = NULL;
 	int comment = 0;
@@ -312,16 +315,13 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 		debug_printf("INST %d\n", inst_num);
 
 		/* fully parsed */
-		tok = match_inst(inst, buf, 0, tmp_result.buf,
-				 sizeof(tmp_result.buf));
+		tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf));
 
 		if (tok > 0) /* we matched at least one token */
 			err = CMDLINE_PARSE_BAD_ARGS;
 
 		else if (!tok) {
 			debug_printf("INST fully parsed\n");
-			memcpy(&result, &tmp_result,
-			       sizeof(result));
 			/* skip spaces */
 			while (isblank2(*curbuf)) {
 				curbuf++;
@@ -332,6 +332,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 				if (!f) {
 					memcpy(&f, &inst->f, sizeof(f));
 					memcpy(&data, &inst->data, sizeof(data));
+					result_ok = result;
 				}
 				else {
 					/* more than 1 inst matches */
@@ -349,6 +350,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* call func */
 	if (f) {
+		result = result_ok;
 		f(result.buf, cl, data);
 	}
 
-- 
2.13.3

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Olivier MATZ]

Hi Xueming,

On Sat, Dec 09, 2017 at 11:39:23PM +0800, Xueming Li wrote:
> Initialize result memory every time before parsing. Also save
> successfully parsed result before further ambiguous command detection to
> avoid result being tainted by later parsing.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

I'm ok with the content of the patch, but this has 2 be split in 2
commits, which fixes different things.

1/ cmdline: fix dynamic tokens parsing

   [contains what Adrien suggested = all your patch but memset]

   When using dynamic tokens, the result buffer contains pointers
   to some location inside the result buffer. When the content of
   the temporary buffer is copied in the final one, these pointers
   still point to the temporary buffer.

   This works until the temporary buffer is kept intact, but the
   next commit introduces a memset() that breaks this assumption.

   This commit renames the buffers, and ensures that the pointers
   point to the valid location, by recopying the buffer before
   invoking f().

   Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
   Cc: stable@dpdk.org


2/ cmdline: avoid garbage in unused fields of parsed result

   [contains the memset() only]

   The result buffer was not initialized before parsing, inducing
   garbage in unused fields or padding of the parsed structure.

   Initialize the result buffer each time before parsing.

   Fixes: af75078fece3 ("first public release")
   Cc: stable@dpdk.org


Thoughts?
Adrien, are you also ok?

Thanks,
Olivier

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Adrien Mazarguil]

On Thu, Dec 14, 2017 at 04:35:45PM +0100, Olivier MATZ wrote:
> Hi Xueming,
> 
> On Sat, Dec 09, 2017 at 11:39:23PM +0800, Xueming Li wrote:
> > Initialize result memory every time before parsing. Also save
> > successfully parsed result before further ambiguous command detection to
> > avoid result being tainted by later parsing.
> > 
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> I'm ok with the content of the patch, but this has 2 be split in 2
> commits, which fixes different things.
> 
> 1/ cmdline: fix dynamic tokens parsing
> 
>    [contains what Adrien suggested = all your patch but memset]
> 
>    When using dynamic tokens, the result buffer contains pointers
>    to some location inside the result buffer. When the content of
>    the temporary buffer is copied in the final one, these pointers
>    still point to the temporary buffer.
> 
>    This works until the temporary buffer is kept intact, but the
>    next commit introduces a memset() that breaks this assumption.
> 
>    This commit renames the buffers, and ensures that the pointers
>    point to the valid location, by recopying the buffer before
>    invoking f().
> 
>    Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
>    Cc: stable@dpdk.org
> 
> 
> 2/ cmdline: avoid garbage in unused fields of parsed result
> 
>    [contains the memset() only]
> 
>    The result buffer was not initialized before parsing, inducing
>    garbage in unused fields or padding of the parsed structure.
> 
>    Initialize the result buffer each time before parsing.
> 
>    Fixes: af75078fece3 ("first public release")
>    Cc: stable@dpdk.org
> 
> 
> Thoughts?
> Adrien, are you also ok?

Yes I fully agree, splitting this in two patches is also what I had in mind.
Xueming, do you plan to submit v3 accordingly?

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Xueming(Steven) Li]

No problem, make enough sense for v3.

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, December 18, 2017 6:51 PM
> To: Olivier MATZ <olivier.matz@6wind.com>
> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> 
> On Thu, Dec 14, 2017 at 04:35:45PM +0100, Olivier MATZ wrote:
> > Hi Xueming,
> >
> > On Sat, Dec 09, 2017 at 11:39:23PM +0800, Xueming Li wrote:
> > > Initialize result memory every time before parsing. Also save
> > > successfully parsed result before further ambiguous command
> > > detection to avoid result being tainted by later parsing.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> >
> > I'm ok with the content of the patch, but this has 2 be split in 2
> > commits, which fixes different things.
> >
> > 1/ cmdline: fix dynamic tokens parsing
> >
> >    [contains what Adrien suggested = all your patch but memset]
> >
> >    When using dynamic tokens, the result buffer contains pointers
> >    to some location inside the result buffer. When the content of
> >    the temporary buffer is copied in the final one, these pointers
> >    still point to the temporary buffer.
> >
> >    This works until the temporary buffer is kept intact, but the
> >    next commit introduces a memset() that breaks this assumption.
> >
> >    This commit renames the buffers, and ensures that the pointers
> >    point to the valid location, by recopying the buffer before
> >    invoking f().
> >
> >    Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
> >    Cc: stable@dpdk.org
> >
> >
> > 2/ cmdline: avoid garbage in unused fields of parsed result
> >
> >    [contains the memset() only]
> >
> >    The result buffer was not initialized before parsing, inducing
> >    garbage in unused fields or padding of the parsed structure.
> >
> >    Initialize the result buffer each time before parsing.
> >
> >    Fixes: af75078fece3 ("first public release")
> >    Cc: stable@dpdk.org
> >
> >
> > Thoughts?
> > Adrien, are you also ok?
> 
> Yes I fully agree, splitting this in two patches is also what I had in
> mind.
> Xueming, do you plan to submit v3 accordingly?
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Xueming(Steven) Li]

HI Olivier,

By reading p1 comments carefully, looks like the pointer to result buffer issue
not resolved by result copy. How about this:

@@ -263,6 +263,7 @@
 #ifdef RTE_LIBRTE_CMDLINE_DEBUG
 	char debug_buf[BUFSIZ];
 #endif
+	char *result_buf = result.buf;
 
 	if (!cl || !buf)
 		return CMDLINE_PARSE_BAD_ARGS;
@@ -312,16 +313,13 @@
 		debug_printf("INST %d\n", inst_num);
 
 		/* fully parsed */
-		tok = match_inst(inst, buf, 0, tmp_result.buf,
-				 sizeof(tmp_result.buf));
+		tok = match_inst(inst, buf, 0, result_buf, sizeof(result.buf));
 
 		if (tok > 0) /* we matched at least one token */
 			err = CMDLINE_PARSE_BAD_ARGS;
 
 		else if (!tok) {
 			debug_printf("INST fully parsed\n");
-			memcpy(&result, &tmp_result,
-			       sizeof(result));
 			/* skip spaces */
 			while (isblank2(*curbuf)) {
 				curbuf++;
@@ -332,6 +330,7 @@
 				if (!f) {
 					memcpy(&f, &inst->f, sizeof(f));
 					memcpy(&data, &inst->data, sizeof(data));
+					result_buf = tmp_result.buf;
 				}
 				else {
 					/* more than 1 inst matches */

Merry Christmas

Xueming(Steven)

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 14, 2017 11:36 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> 
> Hi Xueming,
> 
> On Sat, Dec 09, 2017 at 11:39:23PM +0800, Xueming Li wrote:
> > Initialize result memory every time before parsing. Also save
> > successfully parsed result before further ambiguous command detection
> > to avoid result being tainted by later parsing.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> I'm ok with the content of the patch, but this has 2 be split in 2 commits,
> which fixes different things.
> 
> 1/ cmdline: fix dynamic tokens parsing
> 
>    [contains what Adrien suggested = all your patch but memset]
> 
>    When using dynamic tokens, the result buffer contains pointers
>    to some location inside the result buffer. When the content of
>    the temporary buffer is copied in the final one, these pointers
>    still point to the temporary buffer.
> 
>    This works until the temporary buffer is kept intact, but the
>    next commit introduces a memset() that breaks this assumption.
> 
>    This commit renames the buffers, and ensures that the pointers
>    point to the valid location, by recopying the buffer before
>    invoking f().
> 
>    Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
>    Cc: stable@dpdk.org
> 
> 
> 2/ cmdline: avoid garbage in unused fields of parsed result
> 
>    [contains the memset() only]
> 
>    The result buffer was not initialized before parsing, inducing
>    garbage in unused fields or padding of the parsed structure.
> 
>    Initialize the result buffer each time before parsing.
> 
>    Fixes: af75078fece3 ("first public release")
>    Cc: stable@dpdk.org
> 
> 
> Thoughts?
> Adrien, are you also ok?
> 
> Thanks,
> Olivier

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Olivier Matz]

Hi Xueming,

Sorry for the late response.

On Tue, Dec 26, 2017 at 12:57:41PM +0000, Xueming(Steven) Li wrote:
> HI Olivier,
> 
> By reading p1 comments carefully, looks like the pointer to result buffer issue
> not resolved by result copy. How about this:
> 
> @@ -263,6 +263,7 @@
>  #ifdef RTE_LIBRTE_CMDLINE_DEBUG
>  	char debug_buf[BUFSIZ];
>  #endif
> +	char *result_buf = result.buf;
>  
>  	if (!cl || !buf)
>  		return CMDLINE_PARSE_BAD_ARGS;
> @@ -312,16 +313,13 @@
>  		debug_printf("INST %d\n", inst_num);
>  
>  		/* fully parsed */
> -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> -				 sizeof(tmp_result.buf));
> +		tok = match_inst(inst, buf, 0, result_buf, sizeof(result.buf));

If we don't use tmp_result, it is maybe better to also replace
sizeof(result.buf) by CMDLINE_PARSE_RESULT_BUFSIZE

>  
>  		if (tok > 0) /* we matched at least one token */
>  			err = CMDLINE_PARSE_BAD_ARGS;
>  
>  		else if (!tok) {
>  			debug_printf("INST fully parsed\n");
> -			memcpy(&result, &tmp_result,
> -			       sizeof(result));
>  			/* skip spaces */
>  			while (isblank2(*curbuf)) {
>  				curbuf++;
> @@ -332,6 +330,7 @@
>  				if (!f) {
>  					memcpy(&f, &inst->f, sizeof(f));
>  					memcpy(&data, &inst->data, sizeof(data));
> +					result_buf = tmp_result.buf;
>  				}
>  				else {
>  					/* more than 1 inst matches */
> 


I guess the issue you are talking about is the one described in
"cmdline: fix dynamic tokens parsing" in my previous description?

I think this patch is ok, and is probably better than the initial
suggestion, because it avoids to copy the buffer. However, I don't
understand why the previous patch was wrong, can you detail?

Thanks
Olivier

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Xueming(Steven) Li]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, January 16, 2018 8:46 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> 
> Hi Xueming,
> 
> Sorry for the late response.
> 
> On Tue, Dec 26, 2017 at 12:57:41PM +0000, Xueming(Steven) Li wrote:
> > HI Olivier,
> >
> > By reading p1 comments carefully, looks like the pointer to result
> > buffer issue not resolved by result copy. How about this:
> >
> > @@ -263,6 +263,7 @@
> >  #ifdef RTE_LIBRTE_CMDLINE_DEBUG
> >  	char debug_buf[BUFSIZ];
> >  #endif
> > +	char *result_buf = result.buf;
> >
> >  	if (!cl || !buf)
> >  		return CMDLINE_PARSE_BAD_ARGS;
> > @@ -312,16 +313,13 @@
> >  		debug_printf("INST %d\n", inst_num);
> >
> >  		/* fully parsed */
> > -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> > -				 sizeof(tmp_result.buf));
> > +		tok = match_inst(inst, buf, 0, result_buf, sizeof(result.buf));
> 
> If we don't use tmp_result, it is maybe better to also replace
> sizeof(result.buf) by CMDLINE_PARSE_RESULT_BUFSIZE

Thanks, would like to send out new version soon

> 
> >
> >  		if (tok > 0) /* we matched at least one token */
> >  			err = CMDLINE_PARSE_BAD_ARGS;
> >
> >  		else if (!tok) {
> >  			debug_printf("INST fully parsed\n");
> > -			memcpy(&result, &tmp_result,
> > -			       sizeof(result));
> >  			/* skip spaces */
> >  			while (isblank2(*curbuf)) {
> >  				curbuf++;
> > @@ -332,6 +330,7 @@
> >  				if (!f) {
> >  					memcpy(&f, &inst->f, sizeof(f));
> >  					memcpy(&data, &inst->data, sizeof(data));
> > +					result_buf = tmp_result.buf;
> >  				}
> >  				else {
> >  					/* more than 1 inst matches */
> >
> 
> 
> I guess the issue you are talking about is the one described in
> "cmdline: fix dynamic tokens parsing" in my previous description?
> 
> I think this patch is ok, and is probably better than the initial
> suggestion, because it avoids to copy the buffer. However, I don't
> understand why the previous patch was wrong, can you detail?

Let me quote your last email:
"  When using dynamic tokens, the result buffer contains pointers
   to some location inside the result buffer. When the content of
   the temporary buffer is copied in the final one, these pointers
   still point to the temporary buffer."
If pointer exist in buffer, the new copy still pint to the old copy
which very probably being changed.

> 
> Thanks
> Olivier

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Olivier Matz]

On Thu, Jan 18, 2018 at 04:29:59AM +0000, Xueming(Steven) Li wrote:
> 
> 
> > -----Original Message-----
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Tuesday, January 16, 2018 8:46 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> > Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> > 
> > Hi Xueming,
> > 
> > Sorry for the late response.
> > 
> > On Tue, Dec 26, 2017 at 12:57:41PM +0000, Xueming(Steven) Li wrote:
> > > HI Olivier,
> > >
> > > By reading p1 comments carefully, looks like the pointer to result
> > > buffer issue not resolved by result copy. How about this:
> > >
> > > @@ -263,6 +263,7 @@
> > >  #ifdef RTE_LIBRTE_CMDLINE_DEBUG
> > >  	char debug_buf[BUFSIZ];
> > >  #endif
> > > +	char *result_buf = result.buf;
> > >
> > >  	if (!cl || !buf)
> > >  		return CMDLINE_PARSE_BAD_ARGS;
> > > @@ -312,16 +313,13 @@
> > >  		debug_printf("INST %d\n", inst_num);
> > >
> > >  		/* fully parsed */
> > > -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> > > -				 sizeof(tmp_result.buf));
> > > +		tok = match_inst(inst, buf, 0, result_buf, sizeof(result.buf));
> > 
> > If we don't use tmp_result, it is maybe better to also replace
> > sizeof(result.buf) by CMDLINE_PARSE_RESULT_BUFSIZE
> 
> Thanks, would like to send out new version soon
> 
> > 
> > >
> > >  		if (tok > 0) /* we matched at least one token */
> > >  			err = CMDLINE_PARSE_BAD_ARGS;
> > >
> > >  		else if (!tok) {
> > >  			debug_printf("INST fully parsed\n");
> > > -			memcpy(&result, &tmp_result,
> > > -			       sizeof(result));
> > >  			/* skip spaces */
> > >  			while (isblank2(*curbuf)) {
> > >  				curbuf++;
> > > @@ -332,6 +330,7 @@
> > >  				if (!f) {
> > >  					memcpy(&f, &inst->f, sizeof(f));
> > >  					memcpy(&data, &inst->data, sizeof(data));
> > > +					result_buf = tmp_result.buf;
> > >  				}
> > >  				else {
> > >  					/* more than 1 inst matches */
> > >
> > 
> > 
> > I guess the issue you are talking about is the one described in
> > "cmdline: fix dynamic tokens parsing" in my previous description?
> > 
> > I think this patch is ok, and is probably better than the initial
> > suggestion, because it avoids to copy the buffer. However, I don't
> > understand why the previous patch was wrong, can you detail?
> 
> Let me quote your last email:
> "  When using dynamic tokens, the result buffer contains pointers
>    to some location inside the result buffer. When the content of
>    the temporary buffer is copied in the final one, these pointers
>    still point to the temporary buffer."
> If pointer exist in buffer, the new copy still pint to the old copy
> which very probably being changed.

In patch v2, I still think it was correct because there were 2 copies:

  1/ tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf));

     result is set, it may contain pointers to itself

  2/ result_ok = result;

     result is copied in result_ok
     result_ok may contain pointer to result

  3/ other calls to match_inst(inst, buf, 0, result...)

     this can clobber the result buffer

  4/ result = result_ok;   // before calling f()

     restores the content of result as it was in 1/
     it may contain pointers to itself, which is valid

Was there another problem I missed?


Anyway, I think your v3 is better because it avoids buffer copies.
If it's ok for you, please send a v4 with the small updated regarding
sizeof(result.buf) -> CMDLINE_PARSE_RESULT_BUFSIZE.

Thanks,
Olivier

Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

[Xueming(Steven) Li]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 19, 2018 5:07 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> 
> On Thu, Jan 18, 2018 at 04:29:59AM +0000, Xueming(Steven) Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Tuesday, January 16, 2018 8:46 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> > > Subject: Re: [PATCH v2] lib/cmdline: init CLI parsing memory
> > >
> > > Hi Xueming,
> > >
> > > Sorry for the late response.
> > >
> > > On Tue, Dec 26, 2017 at 12:57:41PM +0000, Xueming(Steven) Li wrote:
> > > > HI Olivier,
> > > >
> > > > By reading p1 comments carefully, looks like the pointer to result
> > > > buffer issue not resolved by result copy. How about this:
> > > >
> > > > @@ -263,6 +263,7 @@
> > > >  #ifdef RTE_LIBRTE_CMDLINE_DEBUG
> > > >  	char debug_buf[BUFSIZ];
> > > >  #endif
> > > > +	char *result_buf = result.buf;
> > > >
> > > >  	if (!cl || !buf)
> > > >  		return CMDLINE_PARSE_BAD_ARGS;
> > > > @@ -312,16 +313,13 @@
> > > >  		debug_printf("INST %d\n", inst_num);
> > > >
> > > >  		/* fully parsed */
> > > > -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> > > > -				 sizeof(tmp_result.buf));
> > > > +		tok = match_inst(inst, buf, 0, result_buf,
> sizeof(result.buf));
> > >
> > > If we don't use tmp_result, it is maybe better to also replace
> > > sizeof(result.buf) by CMDLINE_PARSE_RESULT_BUFSIZE
> >
> > Thanks, would like to send out new version soon
> >
> > >
> > > >
> > > >  		if (tok > 0) /* we matched at least one token */
> > > >  			err = CMDLINE_PARSE_BAD_ARGS;
> > > >
> > > >  		else if (!tok) {
> > > >  			debug_printf("INST fully parsed\n");
> > > > -			memcpy(&result, &tmp_result,
> > > > -			       sizeof(result));
> > > >  			/* skip spaces */
> > > >  			while (isblank2(*curbuf)) {
> > > >  				curbuf++;
> > > > @@ -332,6 +330,7 @@
> > > >  				if (!f) {
> > > >  					memcpy(&f, &inst->f, sizeof(f));
> > > >  					memcpy(&data, &inst->data,
> sizeof(data));
> > > > +					result_buf = tmp_result.buf;
> > > >  				}
> > > >  				else {
> > > >  					/* more than 1 inst matches */
> > > >
> > >
> > >
> > > I guess the issue you are talking about is the one described in
> > > "cmdline: fix dynamic tokens parsing" in my previous description?
> > >
> > > I think this patch is ok, and is probably better than the initial
> > > suggestion, because it avoids to copy the buffer. However, I don't
> > > understand why the previous patch was wrong, can you detail?
> >
> > Let me quote your last email:
> > "  When using dynamic tokens, the result buffer contains pointers
> >    to some location inside the result buffer. When the content of
> >    the temporary buffer is copied in the final one, these pointers
> >    still point to the temporary buffer."
> > If pointer exist in buffer, the new copy still pint to the old copy
> > which very probably being changed.
> 
> In patch v2, I still think it was correct because there were 2 copies:
> 
>   1/ tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf));
> 
>      result is set, it may contain pointers to itself
> 
>   2/ result_ok = result;
> 
>      result is copied in result_ok
>      result_ok may contain pointer to result
> 
>   3/ other calls to match_inst(inst, buf, 0, result...)
> 
>      this can clobber the result buffer
> 
>   4/ result = result_ok;   // before calling f()
> 
>      restores the content of result as it was in 1/
>      it may contain pointers to itself, which is valid

You are correct, I ignored this step, blind hit.

> 
> Was there another problem I missed?
> 
> 
> Anyway, I think your v3 is better because it avoids buffer copies.
> If it's ok for you, please send a v4 with the small updated regarding
> sizeof(result.buf) -> CMDLINE_PARSE_RESULT_BUFSIZE.
> 
> Thanks,
> Olivier

[dpdk-dev] [PATCH v3] cmdline: fix dynamic tokens parsing

[Xueming Li]

When using dynamic tokens, the result buffer contains pointers to some
location inside the result buffer. When the content of the temporary
buffer is copied in the final one, these pointers still point to the
temporary buffer.

This works until the temporary buffer is kept intact, but the next
commit introduces a memset() that breaks this assumption.

This commit keeps the successfully parsed buffers, and ensures that the
pointers point to the valid location, by using temp buffer for following
parsing.

Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
Cc: stable@dpdk.org
Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_cmdline/cmdline_parse.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 3e12ee54f..c74b146fc 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -263,6 +263,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 #ifdef RTE_LIBRTE_CMDLINE_DEBUG
 	char debug_buf[BUFSIZ];
 #endif
+	char *result_buf = result.buf;
 
 	if (!cl || !buf)
 		return CMDLINE_PARSE_BAD_ARGS;
@@ -312,16 +313,14 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 		debug_printf("INST %d\n", inst_num);
 
 		/* fully parsed */
-		tok = match_inst(inst, buf, 0, tmp_result.buf,
-				 sizeof(tmp_result.buf));
+		tok = match_inst(inst, buf, 0, result_buf,
+				 CMDLINE_PARSE_RESULT_BUFSIZE);
 
 		if (tok > 0) /* we matched at least one token */
 			err = CMDLINE_PARSE_BAD_ARGS;
 
 		else if (!tok) {
 			debug_printf("INST fully parsed\n");
-			memcpy(&result, &tmp_result,
-			       sizeof(result));
 			/* skip spaces */
 			while (isblank2(*curbuf)) {
 				curbuf++;
@@ -332,6 +331,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 				if (!f) {
 					memcpy(&f, &inst->f, sizeof(f));
 					memcpy(&data, &inst->data, sizeof(data));
+					result_buf = tmp_result.buf;
 				}
 				else {
 					/* more than 1 inst matches */
-- 
2.13.3

Re: [dpdk-dev] [PATCH v3] cmdline: fix dynamic tokens parsing

[Olivier Matz]

On Sat, Jan 20, 2018 at 02:16:10AM +0800, Xueming Li wrote:
> When using dynamic tokens, the result buffer contains pointers to some
> location inside the result buffer. When the content of the temporary
> buffer is copied in the final one, these pointers still point to the
> temporary buffer.
> 
> This works until the temporary buffer is kept intact, but the next
> commit introduces a memset() that breaks this assumption.
> 
> This commit keeps the successfully parsed buffers, and ensures that the
> pointers point to the valid location, by using temp buffer for following
> parsing.
> 
> Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
> Cc: stable@dpdk.org
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] cmdline: fix dynamic tokens parsing

[Thomas Monjalon]

22/01/2018 14:13, Olivier Matz:
> On Sat, Jan 20, 2018 at 02:16:10AM +0800, Xueming Li wrote:
> > When using dynamic tokens, the result buffer contains pointers to some
> > location inside the result buffer. When the content of the temporary
> > buffer is copied in the final one, these pointers still point to the
> > temporary buffer.
> > 
> > This works until the temporary buffer is kept intact, but the next
> > commit introduces a memset() that breaks this assumption.
> > 
> > This commit keeps the successfully parsed buffers, and ensures that the
> > pointers point to the valid location, by using temp buffer for following
> > parsing.
> > 
> > Fixes: 9b3fbb051d2e ("cmdline: fix parsing")
> > Cc: stable@dpdk.org
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks