| Message ID | 20130711145009.74852147@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/11/2013 12:50 PM, Luiz Capitulino wrote: > 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. I can't say that I tested it either, but... > > 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. s/beforing/before/ > > 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); I definitely agree that the current generated code passes in a non-null errp, and that visit_type_str is a no-op when started in an existing error. > 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); Is that safe even if the failure was after device was parsed, meaning the initial visitor to password was a no-op and there is nothing to deallocate for password? I _think_ this is a correct fix (it means that errors encountered only while doing a dealloc pass are lost, but what errors are you going to encounter in that direction?); but I'd feel more comfortable is someone else more familiar with visitors chimes in. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > scripts/qapi-commands.py | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > +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) Any reason you don't use space after ',' (several instances)?
On Thu, 11 Jul 2013 13:14:21 -0600 Eric Blake <eblake@redhat.com> wrote: > On 07/11/2013 12:50 PM, Luiz Capitulino wrote: > > 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. > > I can't say that I tested it either, but... > > > > > 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. > > s/beforing/before/ I don't know from where that beforing came from. > > > > 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); > > I definitely agree that the current generated code passes in a non-null > errp, and that visit_type_str is a no-op when started in an existing error. > > > 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); > > Is that safe even if the failure was after device was parsed, meaning > the initial visitor to password was a no-op and there is nothing to > deallocate for password? Yes, for this specific case it's safe. The dealloc visitor checks if its object (password in this case) is NULL, and does nothing if it is. Now, I'm not entirely sure if we'll be safe for complex structures, that have many members including nested structures, optionals etc. Although, not being safe is probably a bug. Maybe, the best way of ensuring this is to have test-cases covering those scenarios. > I _think_ this is a correct fix (it means that > errors encountered only while doing a dealloc pass are lost, but what > errors are you going to encounter in that direction?); Or, what could mngt apps even if an error is possible. > but I'd feel more > comfortable is someone else more familiar with visitors chimes in. Me too :) > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > scripts/qapi-commands.py | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > +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) > > Any reason you don't use space after ',' (several instances)? I don't think so.
On 07/11/13 21:14, Eric Blake wrote: > On 07/11/2013 12:50 PM, Luiz Capitulino wrote: >> 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. > > I can't say that I tested it either, but... > >> >> 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. It's a good idea to fix this. > > s/beforing/before/ > >> >> 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); > > I definitely agree that the current generated code passes in a non-null > errp, and that visit_type_str is a no-op when started in an existing error. > >> 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. I agree with that. > 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); > > Is that safe even if the failure was after device was parsed, meaning > the initial visitor to password was a no-op and there is nothing to > deallocate for password? I _think_ this is a correct fix (it means that > errors encountered only while doing a dealloc pass are lost, but what > errors are you going to encounter in that direction?); but I'd feel more > comfortable is someone else more familiar with visitors chimes in. Two points: (a) passing NULL "errp"s to the dealloc traversal also prevents the dealloc traversal to set an error mid-way, and to abort the traversal. However this is perfectly fine, the dealloc traversal (in parts or in entirity) should never fail. (Cf. you can't throw an exception in a C++ destructor -- the destructor could be running as part of exception propagation already.) (b) The generated traversal code, independently of the visitor object, can (should!) deal with *arbitrarily* incomplete trees since commit d195325b05199038b5907fa791729425b9720d21 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Tue Jul 17 16:17:04 2012 +0200 qapi: fix error propagation > >> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> --- >> scripts/qapi-commands.py | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> > >> +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) > > Any reason you don't use space after ',' (several instances)? > With the spaces fixed: Reviewed-by: Laszlo Ersek <lersek@redhat.com>
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); } ''',
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 <lcapitulino@redhat.com> --- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)