[Qemu-devel] [PULL for-2.6 0/3] Fix dangling pointers and error message regressions

[Qemu-devel] [PULL for-2.6 0/3] Fix dangling pointers and error message regressions

[Markus Armbruster]

PATCH 1+2 are simple fixes for dangling pointers to unused stack, and
as such belong into 2.6 if at all possible.

PATCH 3 fixes an error message regression.  The patch is a bit long,
but repetitive.

The following changes since commit f419a626c76bcb26697883af702862e8623056f9:

  usb/uhci: move pid check (2016-04-25 12:05:05 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-error-2016-04-28

for you to fetch changes up to 51b9b478cc238ad23a78ffd713f9c18bbc3907e6:

  qom: -object error messages lost location, restore it (2016-04-28 08:19:36 +0200)

----------------------------------------------------------------
Fix dangling pointers and error message regressions

----------------------------------------------------------------
Markus Armbruster (3):
      QemuOpts: Fix qemu_opts_foreach() dangling location regression
      replay: Fix dangling location bug in replay_configure()
      qom: -object error messages lost location, restore it

 include/qom/object_interfaces.h |  5 +++--
 qemu-img.c                      | 39 +++++++++++----------------------------
 qemu-io.c                       |  3 +--
 qemu-nbd.c                      |  3 +--
 qom/object_interfaces.c         |  4 +++-
 replay/replay.c                 |  3 ++-
 util/qemu-option.c              |  6 +++---
 vl.c                            |  6 ++----
 8 files changed, 26 insertions(+), 43 deletions(-)

-- 
2.5.5

[Qemu-devel] [PULL for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression

[Markus Armbruster]

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>
Message-Id: <1461767349-15329-2-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 util/qemu-option.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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)
-- 
2.5.5

[Qemu-devel] [PULL for-2.6 2/3] replay: Fix dangling location bug in replay_configure()

[Markus Armbruster]

replay_configure() pushes and pops a Location with automatic storage
duration.  Except it fails to pop when -icount parameter "rr" isn't
given.  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.

Broken in commit 890ad55.

I didn't take the time to find a reproducer.

Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1461767349-15329-3-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 replay/replay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/replay/replay.c b/replay/replay.c
index 7c2573a..167fd29 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -275,7 +275,7 @@ void replay_configure(QemuOpts *opts)
     rr = qemu_opt_get(opts, "rr");
     if (!rr) {
         /* Just enabling icount */
-        return;
+        goto out;
     } else if (!strcmp(rr, "record")) {
         mode = REPLAY_MODE_RECORD;
     } else if (!strcmp(rr, "replay")) {
@@ -293,6 +293,7 @@ void replay_configure(QemuOpts *opts)
 
     replay_enable(fname, mode);
 
+out:
     loc_pop(&loc);
 }
 
-- 
2.5.5

[Qemu-devel] [PULL for-2.6 3/3] qom: -object error messages lost location, restore it

[Markus Armbruster]

qemu_opts_foreach() runs its callback with the error location set to
the option's location.  Any errors the callback reports use the
option's location automatically.

Commit 90998d5 moved the actual error reporting from "inside"
qemu_opts_foreach() to after it.  Here's a typical hunk:

	 if (qemu_opts_foreach(qemu_find_opts("object"),
    -                          object_create,
    -                          object_create_initial, NULL)) {
    +                          user_creatable_add_opts_foreach,
    +                          object_create_initial, &err)) {
    +        error_report_err(err);
	     exit(1);
	 }

Before, object_create() reports from within qemu_opts_foreach(), using
the option's location.  Afterwards, we do it after
qemu_opts_foreach(), using whatever location happens to be current
there.  Commonly a "none" location.

This is because Error objects don't have location information.
Problematic.

Reproducer:

    $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar
    qemu-system-x86_64: Property '.foo' not found

Note no location.  This commit restores it:

    qemu-system-x86_64: -object secret,id=foo,foo=bar: Property '.foo' not found

Note that the qemu_opts_foreach() bug just fixed could mask the bug
here: if the location it leaves dangling hasn't been clobbered, yet,
it's the correct one.

Reported-by: Eric Blake <eblake@redhat.com>
Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1461767349-15329-4-git-send-email-armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[Paragraph on Error added to commit message]
---
 include/qom/object_interfaces.h |  5 +++--
 qemu-img.c                      | 39 +++++++++++----------------------------
 qemu-io.c                       |  3 +--
 qemu-nbd.c                      |  3 +--
 qom/object_interfaces.c         |  4 +++-
 vl.c                            |  6 ++----
 6 files changed, 21 insertions(+), 39 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index d579746..8b17f4d 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -140,7 +140,7 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
  * user_creatable_add_opts_foreach:
  * @opaque: a user_creatable_add_opts_predicate callback or NULL
  * @opts: options to create
- * @errp: if an error occurs, a pointer to an area to store the error
+ * @errp: unused
  *
  * An iterator callback to be used in conjunction with
  * the qemu_opts_foreach() method for creating a list of
@@ -148,8 +148,9 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
  *
  * The @opaque parameter can be passed a user_creatable_add_opts_predicate
  * callback to filter which types of object are created during iteration.
+ * When it fails, report the error.
  *
- * Returns: 0 on success, -1 on error
+ * Returns: 0 on success, -1 when an error was reported.
  */
 int user_creatable_add_opts_foreach(void *opaque,
                                     QemuOpts *opts, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 1697762..46f2a6d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -435,8 +435,7 @@ static int img_create(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         goto fail;
     }
 
@@ -598,7 +597,6 @@ static int img_check(int argc, char **argv)
     bool writethrough;
     ImageCheck *check;
     bool quiet = false;
-    Error *local_err = NULL;
     bool image_opts = false;
 
     fmt = NULL;
@@ -679,8 +677,7 @@ static int img_check(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -871,8 +868,7 @@ static int img_commit(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -1133,7 +1129,6 @@ static int img_compare(int argc, char **argv)
     int64_t nb_sectors;
     int c, pnum;
     uint64_t progress_base;
-    Error *local_err = NULL;
     bool image_opts = false;
 
     cache = BDRV_DEFAULT_CACHE;
@@ -1201,8 +1196,7 @@ static int img_compare(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         ret = 2;
         goto out4;
     }
@@ -1864,8 +1858,7 @@ static int img_convert(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         goto fail_getopt;
     }
 
@@ -2299,7 +2292,6 @@ static int img_info(int argc, char **argv)
     bool chain = false;
     const char *filename, *fmt, *output;
     ImageInfoList *list;
-    Error *local_err = NULL;
     bool image_opts = false;
 
     fmt = NULL;
@@ -2363,8 +2355,7 @@ static int img_info(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -2513,7 +2504,6 @@ static int img_map(int argc, char **argv)
     int64_t length;
     MapEntry curr = { .length = 0 }, next;
     int ret = 0;
-    Error *local_err = NULL;
     bool image_opts = false;
 
     fmt = NULL;
@@ -2573,8 +2563,7 @@ static int img_map(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -2717,8 +2706,7 @@ static int img_snapshot(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &err)) {
-        error_report_err(err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -2867,8 +2855,7 @@ static int img_rebase(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -3133,7 +3120,6 @@ static int img_resize(int argc, char **argv)
     bool quiet = false;
     BlockBackend *blk = NULL;
     QemuOpts *param;
-    Error *local_err = NULL;
 
     static QemuOptsList resize_options = {
         .name = "resize_options",
@@ -3204,8 +3190,7 @@ static int img_resize(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         return 1;
     }
 
@@ -3297,7 +3282,6 @@ static int img_amend(int argc, char **argv)
     bool quiet = false, progress = false;
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
-    Error *local_err = NULL;
     bool image_opts = false;
 
     cache = BDRV_DEFAULT_CACHE;
@@ -3365,8 +3349,7 @@ static int img_amend(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         ret = -1;
         goto out_no_progress;
     }
diff --git a/qemu-io.c b/qemu-io.c
index 288bba8..0598251 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -534,8 +534,7 @@ int main(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_error)) {
-        error_report_err(local_error);
+                          NULL, NULL)) {
         exit(1);
     }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2c9754e..c55b40f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -711,8 +711,7 @@ int main(int argc, char **argv)
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
-                          NULL, &local_err)) {
-        error_report_err(local_err);
+                          NULL, NULL)) {
         exit(EXIT_FAILURE);
     }
 
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ab5da35..3931890 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -170,6 +170,7 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
 {
     bool (*type_predicate)(const char *) = opaque;
     Object *obj = NULL;
+    Error *err = NULL;
     const char *type;
 
     type = qemu_opt_get(opts, "qom-type");
@@ -178,8 +179,9 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
         return 0;
     }
 
-    obj = user_creatable_add_opts(opts, errp);
+    obj = user_creatable_add_opts(opts, &err);
     if (!obj) {
+        error_report_err(err);
         return -1;
     }
     object_unref(obj);
diff --git a/vl.c b/vl.c
index 9df534f..5fd22cb 100644
--- a/vl.c
+++ b/vl.c
@@ -4291,8 +4291,7 @@ int main(int argc, char **argv, char **envp)
 
     if (qemu_opts_foreach(qemu_find_opts("object"),
                           user_creatable_add_opts_foreach,
-                          object_create_initial, &err)) {
-        error_report_err(err);
+                          object_create_initial, NULL)) {
         exit(1);
     }
 
@@ -4410,8 +4409,7 @@ int main(int argc, char **argv, char **envp)
 
     if (qemu_opts_foreach(qemu_find_opts("object"),
                           user_creatable_add_opts_foreach,
-                          object_create_delayed, &err)) {
-        error_report_err(err);
+                          object_create_delayed, NULL)) {
         exit(1);
     }
 
-- 
2.5.5

Re: [Qemu-devel] [PULL for-2.6 0/3] Fix dangling pointers and error message regressions

[Peter Maydell]

On 28 April 2016 at 07:28, Markus Armbruster <armbru@redhat.com> wrote:
> PATCH 1+2 are simple fixes for dangling pointers to unused stack, and
> as such belong into 2.6 if at all possible.
>
> PATCH 3 fixes an error message regression.  The patch is a bit long,
> but repetitive.
>
> The following changes since commit f419a626c76bcb26697883af702862e8623056f9:
>
>   usb/uhci: move pid check (2016-04-25 12:05:05 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-error-2016-04-28
>
> for you to fetch changes up to 51b9b478cc238ad23a78ffd713f9c18bbc3907e6:
>
>   qom: -object error messages lost location, restore it (2016-04-28 08:19:36 +0200)
>
> ----------------------------------------------------------------
> Fix dangling pointers and error message regressions
>
> ----------------------------------------------------------------
> Markus Armbruster (3):
>       QemuOpts: Fix qemu_opts_foreach() dangling location regression
>       replay: Fix dangling location bug in replay_configure()
>       qom: -object error messages lost location, restore it
>

Applied, thanks.

-- PMM