Skip to content

Commit

Permalink
qapi: qapi-commands: fix possible leaks on visitor dealloc
Browse files Browse the repository at this point in the history
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, before 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
  • Loading branch information
Luiz Capitulino committed Jul 16, 2013
1 parent 6453a3a commit 8f91ad8
Showing 1 changed file with 10 additions and 7 deletions.
17 changes: 10 additions & 7 deletions scripts/qapi-commands.py
Expand Up @@ -128,12 +128,15 @@ def gen_visitor_input_vars_decl(args):

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);
Expand All @@ -148,22 +151,22 @@ def gen_visitor_input_block(args, obj, dealloc=False):
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('''
Expand Down Expand Up @@ -194,7 +197,7 @@ def gen_marshal_output(name, args, ret_type, middle_mode):
}
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);
}
''',
Expand Down

0 comments on commit 8f91ad8

Please sign in to comment.