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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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