Re: [Qemu-devel] [PATCH] qapi: qapi-commands: fix possible leaks on visitor dealloc

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Luiz Capitulino (2013-07-12 09:42:02)
> 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>

> ---
> 
> o rfc
> 
>  - Fixed missing spaces after ','
>  - Reworded commitlog a bit
> 
>  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..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);
>  }
>  ''',
> -- 
> 1.8.1.4