Message ID | 20130712104202.6a99c3f0@redhat.com |
---|---|
State | New |
Headers | show |
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
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); } ''',