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

  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():
  

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