[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] qapi: qapi-commands: fix possible leaks on visito
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [RFC] qapi: qapi-commands: fix possible leaks on visitor dealloc |
Date: |
Thu, 11 Jul 2013 13:14:21 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
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 <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)?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature