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