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/

> 
> 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?  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?); but I'd feel more
comfortable is someone else more familiar with visitors chimes in.

> 
> 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)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org