qapi: qapi-commands: fix possible leaks on visitor dealloc
RFC 1/1 qapi: qapi-commands: fix possible leaks on visitor dealloc
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);--- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
scripts/qapi-commands.py +10 -7
@@ -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); } ''',
Signature · 1 line
1.8.1.4
Quoted 2 lines · Luiz Capitulino
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...
Quoted 5 lines
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/
Quoted 21 lines
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);
Quoted 4 lines
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.
Quoted 15 lines
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:
Quoted 6 lines
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.
scripts/qapi-commands.py v = qmp_input_get_visitor(mi); · line 157
Quoted 12 lines
Signed-off-by Luiz Capitulino <[email protected]>--- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)scripts/qapi-commands.py +2
-1+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)?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 7 lines · On Thu, 11 Jul 2013 13:14:21 -0600 Eric Blake <[email protected]> wrote:
Quoted 2 lines · Luiz Capitulino
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...
Quoted 1 line
Quoted 6 lines
Quoted 4 lines
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.
Quoted 51 lines
Quoted 21 lines
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);Quoted 4 lines
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_stris a no-op when started in an existing error.Quoted 15 lines
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:
Quoted 6 lines
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);
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
but I'd feel more
comfortable is someone else more familiar with visitors chimes in.
Me too :)
Quoted 8 lines
Quoted 7 lines
Signed-off-by Luiz Capitulino <[email protected]>--- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
Quoted 7 lines
scripts/qapi-commands.py v = qmp_input_get_visitor(mi); · line 157
Quoted 5 lines
scripts/qapi-commands.py +2
-1+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.
Laszlo Ersek
Reviewed-byQuoted 11 lines · Eric Blake
Quoted 2 lines · Luiz Capitulino
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...
Quoted 5 lines
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.
Quoted 24 lines
s/beforing/before/
Quoted 21 lines
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);
Quoted 22 lines
Quoted 4 lines
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_stris a no-op when started in an existing error.Quoted 14 lines
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.
Quoted 9 lines
The above example would then be:
Quoted 7 lines
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);
Quoted 6 lines
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 <[email protected]>
Date: Tue Jul 17 16:17:04 2012 +0200
qapi: fix error propagation
Quoted 8 lines
Quoted 7 lines
Signed-off-by Luiz Capitulino <[email protected]>--- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
Quoted 8 lines
scripts/qapi-commands.py v = qmp_input_get_visitor(mi); · line 157
Quoted 5 lines
scripts/qapi-commands.py +2
-1+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: