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

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

[Luiz Capitulino]

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.

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.

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

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 scripts/qapi-commands.py | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index e06332b..2505437 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -128,12 +128,15 @@ bool has_%(argname)s = false;
 
 def gen_visitor_input_block(args, obj, dealloc=False):
     ret = ""
+    errparg = 'errp'
+
     if len(args) == 0:
         return ret
 
     push_indent()
 
     if dealloc:
+        errparg = 'NULL'
         ret += mcgen('''
 md = qapi_dealloc_visitor_new();
 v = qapi_dealloc_get_visitor(md);
@@ -148,22 +151,22 @@ v = qmp_input_get_visitor(mi);
     for argname, argtype, optional, structured in parse_args(args):
         if optional:
             ret += mcgen('''
-visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp);
+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)
             push_indent()
         ret += mcgen('''
-%(visitor)s(v, &%(c_name)s, "%(name)s", errp);
+%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s);
 ''',
                      c_name=c_var(argname), name=argname, argtype=argtype,
-                     visitor=type_visitor(argtype))
+                     visitor=type_visitor(argtype),errp=errparg)
         if optional:
             pop_indent()
             ret += mcgen('''
 }
-visit_end_optional(v, errp);
-''')
+visit_end_optional(v, %(errp)s);
+''', errp=errparg)
 
     if dealloc:
         ret += mcgen('''
@@ -194,7 +197,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_o
     }
     qmp_output_visitor_cleanup(mo);
     v = qapi_dealloc_get_visitor(md);
-    %(visitor)s(v, &ret_in, "unused", errp);
+    %(visitor)s(v, &ret_in, "unused", NULL);
     qapi_dealloc_visitor_cleanup(md);
 }
 ''',
-- 
1.8.1.4

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3119 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Luiz Capitulino]

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.

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

[Laszlo Ersek]

On 07/11/13 21:14, Eric Blake 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.

It's a good idea to fix this.

> 
> 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.

I agree with that.

> 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.

Two points:

(a) passing NULL "errp"s to the dealloc traversal also prevents the
dealloc traversal to set an error mid-way, and to abort the traversal.
However this is perfectly fine, the dealloc traversal (in parts or in
entirity) should never fail.

(Cf. you can't throw an exception in a C++ destructor -- the destructor
could be running as part of exception propagation already.)

(b) The generated traversal code, independently of the visitor object,
can (should!) deal with *arbitrarily* incomplete trees since

  commit d195325b05199038b5907fa791729425b9720d21
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Tue Jul 17 16:17:04 2012 +0200

      qapi: fix error propagation

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

With the spaces fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>