diff mbox

[27/56] qapi: qapi-commands: fix possible leaks on visitor dealloc

Message ID 1376406680-16302-28-git-send-email-mdroth@linux.vnet.ibm.com
State New
Headers show

Commit Message

Michael Roth Aug. 13, 2013, 3:10 p.m. UTC 
From: Luiz Capitulino <lcapitulino@redhat.com>

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>
(cherry picked from commit 8f91ad8a1b4702966d91ea58cd90bbde1faea1b3)

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 scripts/qapi-commands.py |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
diff mbox

Patch

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index e06332b..b12b696 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);
 }
 ''',