| Message ID | 1461767349-15329-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/27/2016 08:29 AM, Markus Armbruster wrote: > qemu_opts_foreach() pushes and pops a Location with automatic storage > duration. Except it fails to pop when @func() returns non-zero. > cur_loc then points to unused stack space, and will most likely get > clobbered in short order. > > Clobbered cur_loc can make loc_pop() and error_print_loc() crash or > report bogus locations. > > Affects several qemu command line options as well as qemu-img, > qemu-io, qemu-nbd -object, and blkdebug's configuration file. > > Broken in commit a4c7367, v2.4.0. Latent bug means it's not a regression between 2.5 and 2.6, but I agree that if there is time to get this in 2.6, it is worth having. It's a shame that valgrind doesn't catch use of stale stack space. > cur_loc then points to where qemu_opts_foreach()'s Location used to > be, i.e. unused stack space. With optimization, this Location doesn't > get clobbered for me, and also happens to be the correct location. > Without optimization, it does get clobbered in a way that makes > error_report_err() report no location. And that explains why some people were having problems reproducing. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/qemu-option.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/util/qemu-option.c b/util/qemu-option.c index dd9e73d..3467dc2 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1108,19 +1108,19 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, { Location loc; QemuOpts *opts; - int rc; + int rc = 0; loc_push_none(&loc); QTAILQ_FOREACH(opts, &list->head, next) { loc_restore(&opts->loc); rc = func(opaque, opts, errp); if (rc) { - return rc; + break; } assert(!errp || !*errp); } loc_pop(&loc); - return 0; + return rc; } static size_t count_opts_list(QemuOptsList *list)
qemu_opts_foreach() pushes and pops a Location with automatic storage duration. Except it fails to pop when @func() returns non-zero. cur_loc then points to unused stack space, and will most likely get clobbered in short order. Clobbered cur_loc can make loc_pop() and error_print_loc() crash or report bogus locations. Affects several qemu command line options as well as qemu-img, qemu-io, qemu-nbd -object, and blkdebug's configuration file. Broken in commit a4c7367, v2.4.0. Reproducer: $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar main() reports "Property '.foo' not found" like this: if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, object_create_delayed, &err)) { error_report_err(err); exit(1); } cur_loc then points to where qemu_opts_foreach()'s Location used to be, i.e. unused stack space. With optimization, this Location doesn't get clobbered for me, and also happens to be the correct location. Without optimization, it does get clobbered in a way that makes error_report_err() report no location. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/qemu-option.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)