[ovs-dev] [PATCH] valgrind: Fix memory leak at expr_error.

[William Tu]

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 at ovn.org> wrote:

>
> > On Apr 4, 2016, at 2:51 PM, William Tu <u9012063 at 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 at 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
>
>
>