| Message ID | 1459806690-40475-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
> On Apr 4, 2016, at 2:51 PM, William Tu <u9012063@gmail.com> wrote: > > Reported by test case 2015: ovn -- action parsing. > xvasprintf (util.c:164) > expr_error (expr.c:489) > expr_parse_field (expr.c:2910) > action_parse_field (actions.c:287) > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > ovn/lib/actions.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > index 44957c7..a17b5a7 100644 > --- a/ovn/lib/actions.c > +++ b/ovn/lib/actions.c > @@ -288,6 +288,7 @@ action_parse_field(struct action_context *ctx, > &prereqs); > if (error) { > action_error(ctx, "%s", error); > + free(error); > return false; > } Unless I'm reading it wrong, the original code seems a little funky. "error" is set by expr_parse_field() as the value of "ctx->error". "ctx" is then handed to action_error() which won't do anything since "ctx->error" is not NULL. It seems like this call to action_error() is not necessary at all. Back to your fix, if there is a problem, it seems like action_force_match() would have similar problems to action_parse_field(); they're only called by parse_action(). You're essentially freeing "ctx->error", but isn't it needed by parse_action()? --Justin
Hi Justin, Thanks for the feedback. I think the original code is OK, although a little misleading. At action_parse_field(), the address of *ctx is not passed into expr_parse_field. The expr_parse_field has a ctx as local variable on its stack. So when it returns, the 'error' contains a newly malloc char* and error != ctx->error. It is when calling action_error(), then the 'ctx->error' is NULL and gets a copy of 'error', so we need to free(error). Regards, William On Tue, Apr 5, 2016 at 3:08 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Apr 4, 2016, at 2:51 PM, William Tu <u9012063@gmail.com> wrote: > > > > Reported by test case 2015: ovn -- action parsing. > > xvasprintf (util.c:164) > > expr_error (expr.c:489) > > expr_parse_field (expr.c:2910) > > action_parse_field (actions.c:287) > > > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > ovn/lib/actions.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > > index 44957c7..a17b5a7 100644 > > --- a/ovn/lib/actions.c > > +++ b/ovn/lib/actions.c > > @@ -288,6 +288,7 @@ action_parse_field(struct action_context *ctx, > > &prereqs); > > if (error) { > > action_error(ctx, "%s", error); > > + free(error); > > return false; > > } > > Unless I'm reading it wrong, the original code seems a little funky. > "error" is set by expr_parse_field() as the value of "ctx->error". "ctx" > is then handed to action_error() which won't do anything since "ctx->error" > is not NULL. It seems like this call to action_error() is not necessary at > all. > > Back to your fix, if there is a problem, it seems like > action_force_match() would have similar problems to action_parse_field(); > they're only called by parse_action(). You're essentially freeing > "ctx->error", but isn't it needed by parse_action()? > > --Justin > > >
You're right; I was thinking that "ctx" was being handed to "expr_parse_field". I pushed the change to master. Thanks! --Justin > On Apr 5, 2016, at 3:51 PM, William Tu <u9012063@gmail.com> wrote: > > Hi Justin, > Thanks for the feedback. I think the original code is OK, although a little misleading. > > At action_parse_field(), the address of *ctx is not passed into expr_parse_field. The expr_parse_field has a ctx as local variable on its stack. So when it returns, the 'error' contains a newly malloc char* and error != ctx->error. > > It is when calling action_error(), then the 'ctx->error' is NULL and gets a copy of 'error', so we need to free(error). > > Regards, > William > > On Tue, Apr 5, 2016 at 3:08 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Apr 4, 2016, at 2:51 PM, William Tu <u9012063@gmail.com> wrote: > > > > Reported by test case 2015: ovn -- action parsing. > > xvasprintf (util.c:164) > > expr_error (expr.c:489) > > expr_parse_field (expr.c:2910) > > action_parse_field (actions.c:287) > > > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > ovn/lib/actions.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > > index 44957c7..a17b5a7 100644 > > --- a/ovn/lib/actions.c > > +++ b/ovn/lib/actions.c > > @@ -288,6 +288,7 @@ action_parse_field(struct action_context *ctx, > > &prereqs); > > if (error) { > > action_error(ctx, "%s", error); > > + free(error); > > return false; > > } > > Unless I'm reading it wrong, the original code seems a little funky. "error" is set by expr_parse_field() as the value of "ctx->error". "ctx" is then handed to action_error() which won't do anything since "ctx->error" is not NULL. It seems like this call to action_error() is not necessary at all. > > Back to your fix, if there is a problem, it seems like action_force_match() would have similar problems to action_parse_field(); they're only called by parse_action(). You're essentially freeing "ctx->error", but isn't it needed by parse_action()? > > --Justin > > >
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index 44957c7..a17b5a7 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -288,6 +288,7 @@ action_parse_field(struct action_context *ctx, &prereqs); if (error) { action_error(ctx, "%s", error); + free(error); return false; }
Reported by test case 2015: ovn -- action parsing. xvasprintf (util.c:164) expr_error (expr.c:489) expr_parse_field (expr.c:2910) action_parse_field (actions.c:287) Signed-off-by: William Tu <u9012063@gmail.com> --- ovn/lib/actions.c | 1 + 1 file changed, 1 insertion(+)