Re: [Qemu-devel] [RFC] qapi: qapi-commands: fix possible leaks on visitor dealloc

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 11 Jul 2013 13:14:21 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/11/2013 12:50 PM, Luiz Capitulino wrote:
> > I'm sending this as an RFC because this is untested, and also because
> > I'm wondering if I'm seeing things after a long patch review session.
> 
> I can't say that I tested it either, but...
> 
> > 
> > The problem is: in qmp-marshal.c, the dealloc visitor calls use the
> > same errp pointer of the input visitor calls. This means that if
> > any of the input visitor calls fails, then the dealloc visitor will
> > return early, beforing freeing the object's memory.
> 
> s/beforing/before/

I don't know from where that beforing came from.

> > 
> > Here's an example, consider this code:
> > 
> > int qmp_marshal_input_block_passwd(Monitor *mon, const QDict *qdict, QObject **ret)
> > {
> > 	[...]
> > 
> >     char * device = NULL;
> >     char * password = NULL;
> > 
> >     mi = qmp_input_visitor_new_strict(QOBJECT(args));
> >     v = qmp_input_get_visitor(mi);
> >     visit_type_str(v, &device, "device", errp);
> >     visit_type_str(v, &password, "password", errp);
> >     qmp_input_visitor_cleanup(mi);
> > 
> >     if (error_is_set(errp)) {
> >         goto out;
> >     }
> >     qmp_block_passwd(device, password, errp);
> > 
> > out:
> >     md = qapi_dealloc_visitor_new();
> >     v = qapi_dealloc_get_visitor(md);
> >     visit_type_str(v, &device, "device", errp);
> 
> I definitely agree that the current generated code passes in a non-null
> errp, and that visit_type_str is a no-op when started in an existing error.
> 
> >     visit_type_str(v, &password, "password", errp);
> >     qapi_dealloc_visitor_cleanup(md);
> > 
> > 	[...]
> > 
> >     return 0;
> > }
> > 
> > Consider errp != NULL when the out label is reached, we're going
> > to leak device and password.
> > 
> > This patch fixes this by always passing errp=NULL for dealloc
> > visitors, meaning that we always try to free them regardless of
> > any previous failure. The above example would then be:
> > 
> > out:
> >     md = qapi_dealloc_visitor_new();
> >     v = qapi_dealloc_get_visitor(md);
> >     visit_type_str(v, &device, "device", NULL);
> >     visit_type_str(v, &password, "password", NULL);
> >     qapi_dealloc_visitor_cleanup(md);
> 
> Is that safe even if the failure was after device was parsed, meaning
> the initial visitor to password was a no-op and there is nothing to
> deallocate for password?

Yes, for this specific case it's safe. The dealloc visitor checks
if its object (password in this case) is NULL, and does nothing if it is.

Now, I'm not entirely sure if we'll be safe for complex structures,
that have many members including nested structures, optionals etc.
Although, not being safe is probably a bug.

Maybe, the best way of ensuring this is to have test-cases covering
those scenarios.

> I _think_ this is a correct fix (it means that
> errors encountered only while doing a dealloc pass are lost, but what
> errors are you going to encounter in that direction?);

Or, what could mngt apps even if an error is possible.

> but I'd feel more
> comfortable is someone else more familiar with visitors chimes in.

Me too :)

> 
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  scripts/qapi-commands.py | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> 
> > +visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
> >  if (has_%(c_name)s) {
> >  ''',
> > -                         c_name=c_var(argname), name=argname)
> > +                         c_name=c_var(argname), name=argname,errp=errparg)
> 
> Any reason you don't use space after ',' (several instances)?

I don't think so.