[dpdk-dev,v2] lib/cmdline: init CLI parsing memory
Checks
Commit Message
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(-)
Comments
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
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?
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
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
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
> -----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
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
> -----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
@@ -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);
}