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 <address@hidden>
  Date:   Tue Jul 17 16:17:04 2012 +0200

      qapi: fix error propagation

> 
>>
>> Signed-off-by: Luiz Capitulino <address@hidden>
>> ---
>>  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 <address@hidden>