[PATCH v2 00/14] Miscellaneous error handling fixes

[PATCH v2 00/14] Miscellaneous error handling fixes

[Markus Armbruster]

Maintainers decide whether any of these are serious enough to still go
into 5.0.  I doubt it.

v2:
* PATCH 12-14: New

Markus Armbruster (14):
  cryptodev: Fix cryptodev_builtin_cleanup() error API violation
  block/file-posix: Fix check_cache_dropped() error handling
  cpus: Fix configure_icount() error API violation
  cpus: Proper range-checking for -icount shift=N
  arm/virt: Fix virt_machine_device_plug_cb() error API violation
  fdc: Fix fallback=auto error handling
  bochs-display: Fix vgamem=SIZE error handling
  virtio-net: Fix duplex=... and speed=... error handling
  xen/pt: Fix flawed conversion to realize()
  io: Fix qio_channel_socket_close() error handling
  migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
  tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff
  qga: Fix qmp_guest_get_memory_blocks() error handling
  qga: Fix qmp_guest_suspend_{disk,ram}() error handling

 backends/cryptodev-builtin.c | 10 ++-----
 block/file-posix.c           |  5 +++-
 cpus.c                       | 52 +++++++++++++++++++++---------------
 hw/arm/virt.c                |  4 +--
 hw/block/fdc.c               |  1 +
 hw/display/bochs-display.c   |  6 +++--
 hw/net/virtio-net.c          |  5 +++-
 hw/xen/xen_pt.c              | 12 ++++-----
 io/channel-socket.c          |  5 ++--
 migration/colo.c             |  8 +++++-
 qga/commands-posix.c         |  3 +++
 qga/commands-win32.c         | 14 ++++++++++
 tests/test-logging.c         |  4 +--
 13 files changed, 82 insertions(+), 47 deletions(-)

-- 
2.21.1

[PATCH v2 01/14] cryptodev: Fix cryptodev_builtin_cleanup() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

cryptodev_builtin_cleanup() passes @errp to
cryptodev_builtin_sym_close_session() in a loop.  Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail.  Fix it
anyway.

Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 backends/cryptodev-builtin.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index c8ae3b9742..14316333fe 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -282,12 +282,7 @@ static int cryptodev_builtin_sym_close_session(
     CryptoDevBackendBuiltin *builtin =
                       CRYPTODEV_BACKEND_BUILTIN(backend);
 
-    if (session_id >= MAX_NUM_SESSIONS ||
-              builtin->sessions[session_id] == NULL) {
-        error_setg(errp, "Cannot find a valid session id: %" PRIu64 "",
-                      session_id);
-        return -1;
-    }
+    assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
 
     qcrypto_cipher_free(builtin->sessions[session_id]->cipher);
     g_free(builtin->sessions[session_id]);
@@ -356,8 +351,7 @@ static void cryptodev_builtin_cleanup(
 
     for (i = 0; i < MAX_NUM_SESSIONS; i++) {
         if (builtin->sessions[i] != NULL) {
-            cryptodev_builtin_sym_close_session(
-                    backend, i, 0, errp);
+            cryptodev_builtin_sym_close_session(backend, i, 0, &error_abort);
         }
     }
 
-- 
2.21.1

[PATCH v2 02/14] block/file-posix: Fix check_cache_dropped() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

check_cache_dropped() calls error_setg() in a loop.  It fails to break
the loop in one instance.  If a subsequent iteration error_setg()s
again, it trips error_setv()'s assertion.

Fix it to break the loop.

Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/file-posix.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 7e19bbff5f..094e3b0212 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2691,10 +2691,13 @@ static void check_cache_dropped(BlockDriverState *bs, Error **errp)
         vec_end = DIV_ROUND_UP(length, page_size);
         for (i = 0; i < vec_end; i++) {
             if (vec[i] & 0x1) {
-                error_setg(errp, "page cache still in use!");
                 break;
             }
         }
+        if (i < vec_end) {
+            error_setg(errp, "page cache still in use!");
+            break;
+        }
     }
 
     if (window) {
-- 
2.21.1

[PATCH v2 03/14] cpus: Fix configure_icount() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

configure_icount() is wrong that way.  Harmless, because its @errp is
always &error_abort or &error_fatal.

Just as wrong (and just as harmless): when it fails, it can still
update global state.

Fix all that.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cpus.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/cpus.c b/cpus.c
index ef441bdf62..1b542b37f9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -797,40 +797,49 @@ void cpu_ticks_init(void)
 
 void configure_icount(QemuOpts *opts, Error **errp)
 {
-    const char *option;
+    const char *option = qemu_opt_get(opts, "shift");
+    bool sleep = qemu_opt_get_bool(opts, "sleep", true);
+    bool align = qemu_opt_get_bool(opts, "align", false);
+    long time_shift = -1;
     char *rem_str = NULL;
 
-    option = qemu_opt_get(opts, "shift");
-    if (!option) {
-        if (qemu_opt_get(opts, "align") != NULL) {
-            error_setg(errp, "Please specify shift option when using align");
-        }
+    if (!option && qemu_opt_get(opts, "align")) {
+        error_setg(errp, "Please specify shift option when using align");
         return;
     }
 
-    icount_sleep = qemu_opt_get_bool(opts, "sleep", true);
+    if (align && !sleep) {
+        error_setg(errp, "align=on and sleep=off are incompatible");
+        return;
+    }
+
+    if (strcmp(option, "auto") != 0) {
+        errno = 0;
+        time_shift = strtol(option, &rem_str, 0);
+        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+            error_setg(errp, "icount: Invalid shift value");
+            return;
+        }
+    } else if (icount_align_option) {
+        error_setg(errp, "shift=auto and align=on are incompatible");
+        return;
+    } else if (!icount_sleep) {
+        error_setg(errp, "shift=auto and sleep=off are incompatible");
+        return;
+    }
+
+    icount_sleep = sleep;
     if (icount_sleep) {
         timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
                                          icount_timer_cb, NULL);
     }
 
-    icount_align_option = qemu_opt_get_bool(opts, "align", false);
+    icount_align_option = align;
 
-    if (icount_align_option && !icount_sleep) {
-        error_setg(errp, "align=on and sleep=off are incompatible");
-    }
-    if (strcmp(option, "auto") != 0) {
-        errno = 0;
-        timers_state.icount_time_shift = strtol(option, &rem_str, 0);
-        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
-            error_setg(errp, "icount: Invalid shift value");
-        }
+    if (time_shift >= 0) {
+        timers_state.icount_time_shift = time_shift;
         use_icount = 1;
         return;
-    } else if (icount_align_option) {
-        error_setg(errp, "shift=auto and align=on are incompatible");
-    } else if (!icount_sleep) {
-        error_setg(errp, "shift=auto and sleep=off are incompatible");
     }
 
     use_icount = 2;
-- 
2.21.1

[PATCH v2 04/14] cpus: Proper range-checking for -icount shift=N

[Markus Armbruster]

timers_state.icount_time_shift must be in [0,63] to avoid undefined
behavior when shifting by it, e.g. in cpu_icount_to_ns().
icount_adjust() clamps it to [0,MAX_ICOUNT_SHIFT], with
MAX_ICOUNT_SHIFT = 10.  configure_icount() doesn't.  Fix that.

Fixes: a8bfac37085c3372366d722f131a7e18d664ee4d
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cpus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index 1b542b37f9..5670c96bcf 100644
--- a/cpus.c
+++ b/cpus.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/config-file.h"
+#include "qemu/cutils.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -801,7 +802,6 @@ void configure_icount(QemuOpts *opts, Error **errp)
     bool sleep = qemu_opt_get_bool(opts, "sleep", true);
     bool align = qemu_opt_get_bool(opts, "align", false);
     long time_shift = -1;
-    char *rem_str = NULL;
 
     if (!option && qemu_opt_get(opts, "align")) {
         error_setg(errp, "Please specify shift option when using align");
@@ -814,9 +814,8 @@ void configure_icount(QemuOpts *opts, Error **errp)
     }
 
     if (strcmp(option, "auto") != 0) {
-        errno = 0;
-        time_shift = strtol(option, &rem_str, 0);
-        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+        if (qemu_strtol(option, NULL, 0, &time_shift) < 0
+            || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
             error_setg(errp, "icount: Invalid shift value");
             return;
         }
-- 
2.21.1

[PATCH v2 05/14] arm/virt: Fix virt_machine_device_plug_cb() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

virt_machine_device_plug_cb() passes @errp to
cryptodev_builtin_sym_close_session() in a loop.  Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail.  Fix by
dropping its Error ** parameter.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72..cca5316256 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1186,7 +1186,7 @@ static void create_smmu(const VirtMachineState *vms,
     g_free(node);
 }
 
-static void create_virtio_iommu_dt_bindings(VirtMachineState *vms, Error **errp)
+static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
 {
     const char compat[] = "virtio,pci-iommu";
     uint16_t bdf = vms->virtio_iommu_bdf;
@@ -2118,7 +2118,7 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
 
         vms->iommu = VIRT_IOMMU_VIRTIO;
         vms->virtio_iommu_bdf = pci_get_bdf(pdev);
-        create_virtio_iommu_dt_bindings(vms, errp);
+        create_virtio_iommu_dt_bindings(vms);
     }
 }
 
-- 
2.21.1

[PATCH v2 06/14] fdc: Fix fallback=auto error handling

[Markus Armbruster]

fdctrl_realize_common() rejects fallback=auto.  Used by devices
"isa-fdc", "sysbus-fdc", "SUNW,fdtwo".  The error handling is broken:

    $ qemu-system-x86_64 -nodefaults -device isa-fdc,fallback=auto,driveA=fd0 -drive if=none,id=fd0
    **
    ERROR:/work/armbru/qemu/hw/block/fdc.c:434:pick_drive_type: assertion failed: (drv->drive != FLOPPY_DRIVE_TYPE_AUTO)
    Aborted (core dumped)

Cause: fdctrl_realize_common() neglects to bail out after setting the
error.  Fix that.

Fixes: a73275dd6fc3bfda33165bebc28e0c33c20cb0a0
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 33bc9e2f92..9628cc171e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2615,6 +2615,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
 
     if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
         error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
+        return;
     }
 
     /* Fill 'command_to_handler' lookup table */
-- 
2.21.1

[PATCH v2 07/14] bochs-display: Fix vgamem=SIZE error handling

[Markus Armbruster]

bochs_display_realize() rejects out-of-range vgamem.  The error
handling is broken:

    $ qemu-system-x86_64 -S -display none -monitor stdio
    QEMU 4.2.93 monitor - type 'help' for more information
    (qemu) device_add bochs-display,vgamem=1
    Error: bochs-display: video memory too small
    (qemu) device_add bochs-display,vgamem=1
    RAMBlock "0000:00:04.0/bochs-display-vram" already registered, abort!
    Aborted (core dumped)

Cause: bochs_display_realize() neglects to bail out after setting the
error.  Fix that.

Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/bochs-display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 70eb619ef4..e763a0a72d 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -267,16 +267,18 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     Object *obj = OBJECT(dev);
     int ret;
 
-    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
-
     if (s->vgamem < 4 * MiB) {
         error_setg(errp, "bochs-display: video memory too small");
+        return;
     }
     if (s->vgamem > 256 * MiB) {
         error_setg(errp, "bochs-display: video memory too big");
+        return;
     }
     s->vgamem = pow2ceil(s->vgamem);
 
+    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
+
     memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
                            &error_fatal);
     memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
-- 
2.21.1

Re: [PATCH v2 07/14] bochs-display: Fix vgamem=SIZE error handling

[Gerd Hoffmann]

On Wed, Apr 22, 2020 at 03:07:12PM +0200, Markus Armbruster wrote:
> bochs_display_realize() rejects out-of-range vgamem.  The error
> handling is broken:
> 
>     $ qemu-system-x86_64 -S -display none -monitor stdio
>     QEMU 4.2.93 monitor - type 'help' for more information
>     (qemu) device_add bochs-display,vgamem=1
>     Error: bochs-display: video memory too small
>     (qemu) device_add bochs-display,vgamem=1
>     RAMBlock "0000:00:04.0/bochs-display-vram" already registered, abort!
>     Aborted (core dumped)
> 
> Cause: bochs_display_realize() neglects to bail out after setting the
> error.  Fix that.
> 
> Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

[PATCH v2 08/14] virtio-net: Fix duplex=... and speed=... error handling

[Markus Armbruster]

virtio_net_device_realize() rejects invalid duplex and speed values.
The error handling is broken:

    $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none -monitor stdio
    QEMU 4.2.93 monitor - type 'help' for more information
    (qemu) device_add virtio-net,duplex=x
    Error: 'duplex' must be 'half' or 'full'
    (qemu) c
    =================================================================
    ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 0x62e000014590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
    READ of size 8 at 0x62e000014590 thread T0
	#0 0x560b75c8dc12 in object_dynamic_cast_assert /work/armbru/qemu/qom/object.c:826
	#1 0x560b74c38ac0 in virtio_vmstate_change /work/armbru/qemu/hw/virtio/virtio.c:3210
	#2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
	#3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
	#4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
	#5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
	#6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
	#7 0x560b75a799f2 in handle_hmp_command /work/armbru/qemu/monitor/hmp.c:1082
    [...]

    0x62e000014590 is located 33168 bytes inside of 42288-byte region [0x62e00000c400,0x62e000016930)
    freed by thread T1 here:
	#0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
	#1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
	#2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
	#3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
	#4 0x560b74999fa4 in do_address_space_destroy /work/armbru/qemu/memory.c:2788
	#5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
	#6 0x560b761c8884 in qemu_thread_start /work/armbru/qemu/util/qemu-thread-posix.c:519
	#7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)

    previously allocated by thread T0 here:
	#0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
	#1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
	#2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
	#3 0x560b75010ad9 in qdev_device_add /work/armbru/qemu/qdev-monitor.c:654
	#4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
	#5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
    [...]
    ==15654==ABORTING

Cause: virtio_net_device_realize() neglects to bail out after setting
the error.  Fix that.

Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/virtio-net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a46e3b37a7..b52ff4ab63 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
             n->net_conf.duplex = DUPLEX_FULL;
         } else {
             error_setg(errp, "'duplex' must be 'half' or 'full'");
+            return;
         }
         n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
     } else {
@@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
 
     if (n->net_conf.speed < SPEED_UNKNOWN) {
         error_setg(errp, "'speed' must be between 0 and INT_MAX");
-    } else if (n->net_conf.speed >= 0) {
+        return;
+    }
+    if (n->net_conf.speed >= 0) {
         n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
     }
 
-- 
2.21.1

Re: [PATCH v2 08/14] virtio-net: Fix duplex=... and speed=... error handling

[Michael S. Tsirkin]

On Wed, Apr 22, 2020 at 03:07:13PM +0200, Markus Armbruster wrote:
> virtio_net_device_realize() rejects invalid duplex and speed values.
> The error handling is broken:
> 
>     $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none -monitor stdio
>     QEMU 4.2.93 monitor - type 'help' for more information
>     (qemu) device_add virtio-net,duplex=x
>     Error: 'duplex' must be 'half' or 'full'
>     (qemu) c
>     =================================================================
>     ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 0x62e000014590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
>     READ of size 8 at 0x62e000014590 thread T0
> 	#0 0x560b75c8dc12 in object_dynamic_cast_assert /work/armbru/qemu/qom/object.c:826
> 	#1 0x560b74c38ac0 in virtio_vmstate_change /work/armbru/qemu/hw/virtio/virtio.c:3210
> 	#2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
> 	#3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
> 	#4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
> 	#5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
> 	#6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
> 	#7 0x560b75a799f2 in handle_hmp_command /work/armbru/qemu/monitor/hmp.c:1082
>     [...]
> 
>     0x62e000014590 is located 33168 bytes inside of 42288-byte region [0x62e00000c400,0x62e000016930)
>     freed by thread T1 here:
> 	#0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
> 	#1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
> 	#2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
> 	#3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
> 	#4 0x560b74999fa4 in do_address_space_destroy /work/armbru/qemu/memory.c:2788
> 	#5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
> 	#6 0x560b761c8884 in qemu_thread_start /work/armbru/qemu/util/qemu-thread-posix.c:519
> 	#7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)
> 
>     previously allocated by thread T0 here:
> 	#0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
> 	#1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
> 	#2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
> 	#3 0x560b75010ad9 in qdev_device_add /work/armbru/qemu/qdev-monitor.c:654
> 	#4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
> 	#5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
>     [...]
>     ==15654==ABORTING
> 
> Cause: virtio_net_device_realize() neglects to bail out after setting
> the error.  Fix that.
> 
> Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge with the rest of the patchset.

> ---
>  hw/net/virtio-net.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a46e3b37a7..b52ff4ab63 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>              n->net_conf.duplex = DUPLEX_FULL;
>          } else {
>              error_setg(errp, "'duplex' must be 'half' or 'full'");
> +            return;
>          }
>          n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>      } else {
> @@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>  
>      if (n->net_conf.speed < SPEED_UNKNOWN) {
>          error_setg(errp, "'speed' must be between 0 and INT_MAX");
> -    } else if (n->net_conf.speed >= 0) {
> +        return;
> +    }
> +    if (n->net_conf.speed >= 0) {
>          n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>      }
>  
> -- 
> 2.21.1

[PATCH v2 09/14] xen/pt: Fix flawed conversion to realize()

[Markus Armbruster]

The conversion of xen_pt_initfn() to xen_pt_realize() blindly replaced
XEN_PT_ERR() by error_setg().  Several error conditions that did not
fail xen_pt_initfn() now fail xen_pt_realize().  Unsurprisingly, the
cleanup on these errors looks highly suspicious.

Revert the inappropriate replacements.

Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 hw/xen/xen_pt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index b91082cb8b..81d5ad8da7 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -858,8 +858,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
 
     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
     if (rc < 0) {
-        error_setg_errno(errp, errno, "Mapping machine irq %u to"
-                         " pirq %i failed", machine_irq, pirq);
+        XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+                   machine_irq, pirq, errno);
 
         /* Disable PCI intx assertion (turn on bit10 of devctl) */
         cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -880,8 +880,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
                                        PCI_SLOT(d->devfn),
                                        e_intx);
         if (rc < 0) {
-            error_setg_errno(errp, errno, "Binding of interrupt %u failed",
-                             e_intx);
+            XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+                       e_intx, errno);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -889,8 +889,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
 
             if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
                 if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-                    error_setg_errno(errp, errno, "Unmapping of machine"
-                            " interrupt %u failed", machine_irq);
+                    XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
+                               " (err: %d)\n", machine_irq, errno);
                 }
             }
             s->machine_irq = 0;
-- 
2.21.1

[PATCH v2 10/14] io: Fix qio_channel_socket_close() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qio_channel_socket_close() passes @errp first to
socket_listen_cleanup(), and then, if closesocket() fails, to
error_setg_errno().  If socket_listen_cleanup() failed, this will trip
the assertion in error_setv().

Fix by ignoring a second error.

Fixes: 73564c407caedf992a1c688b5fea776a8b56ba2a
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-socket.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index b74f5b92a0..e1b4667087 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -704,6 +704,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
     int rc = 0;
+    Error *err = NULL;
 
     if (sioc->fd != -1) {
 #ifdef WIN32
@@ -715,8 +716,8 @@ qio_channel_socket_close(QIOChannel *ioc,
 
         if (closesocket(sioc->fd) < 0) {
             sioc->fd = -1;
-            error_setg_errno(errp, errno,
-                             "Unable to close socket");
+            error_setg_errno(&err, errno, "Unable to close socket");
+            error_propagate(errp, err);
             return -1;
         }
         sioc->fd = -1;
-- 
2.21.1

[PATCH v2 11/14] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qmp_xen_colo_do_checkpoint() passes @errp first to
replication_do_checkpoint_all(), and then to
colo_notify_filters_event().  If both fail, this will trip the
assertion in error_setv().

Similar code in secondary_vm_do_failover() calls
colo_notify_filters_event() only after replication_do_checkpoint_all()
succeeded.  Do the same here.

Fixes: 0e8818f023616677416840d6ddc880db8de3c967
Cc: Zhang Chen <chen.zhang@intel.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/colo.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index a54ac84f41..1b3493729b 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -263,7 +263,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
 
 void qmp_xen_colo_do_checkpoint(Error **errp)
 {
-    replication_do_checkpoint_all(errp);
+    Error *err = NULL;
+
+    replication_do_checkpoint_all(&err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     /* Notify all filters of all NIC to do checkpoint */
     colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
 }
-- 
2.21.1

[PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Markus Armbruster]

Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-logging.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6387e4933f..8580b82420 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -73,10 +73,10 @@ static void test_parse_range(void)
     g_assert(qemu_log_in_addr_range(UINT64_MAX));
     g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
 
-    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
+    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);
     g_assert(qemu_log_in_addr_range(0));
     g_assert(qemu_log_in_addr_range(UINT64_MAX));
- 
+
     qemu_set_dfilter_ranges("2..1", &err);
     error_free_or_abort(&err);
 
-- 
2.21.1

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Philippe Mathieu-Daudé]

Hi Markus,

On 4/22/20 3:07 PM, Markus Armbruster wrote:
> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-logging.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index 6387e4933f..8580b82420 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>       g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>   
> -    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
> +    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);

Why sometime use this form, ...

>       g_assert(qemu_log_in_addr_range(0));
>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
> -
> +
>       qemu_set_dfilter_ranges("2..1", &err);
>       error_free_or_abort(&err);

... and then this other form?

>   
> 

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Eric Blake]

On 4/22/20 8:35 AM, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-logging.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/test-logging.c b/tests/test-logging.c
>> index 6387e4933f..8580b82420 100644
>> --- a/tests/test-logging.c
>> +++ b/tests/test-logging.c
>> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>       g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>> -    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
>> +    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);
> 
> Why sometime use this form, ...

This call must not produce an error (if it does, the test aborts, 
proving we had a bug).

> 
>>       g_assert(qemu_log_in_addr_range(0));
>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>> -
>> +
>>       qemu_set_dfilter_ranges("2..1", &err);
>>       error_free_or_abort(&err);
> 
> ... and then this other form?

This call must produce an error (if we do not diagnose the caller's 
error, our code is buggy, and the test must fail)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Philippe Mathieu-Daudé]

On 4/22/20 4:49 PM, Eric Blake wrote:
> On 4/22/20 8:35 AM, Philippe Mathieu-Daudé wrote:
>> Hi Markus,
>>
>> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>>> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   tests/test-logging.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/test-logging.c b/tests/test-logging.c
>>> index 6387e4933f..8580b82420 100644
>>> --- a/tests/test-logging.c
>>> +++ b/tests/test-logging.c
>>> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>>       g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>>> -    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
>>> +    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);
>>
>> Why sometime use this form, ...
> 
> This call must not produce an error (if it does, the test aborts, 
> proving we had a bug).
> 
>>
>>>       g_assert(qemu_log_in_addr_range(0));
>>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>> -
>>> +
>>>       qemu_set_dfilter_ranges("2..1", &err);
>>>       error_free_or_abort(&err);
>>
>> ... and then this other form?
> 
> This call must produce an error (if we do not diagnose the caller's 
> error, our code is buggy, and the test must fail)

Ah OK it makes sense, thanks!

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Markus,
>
> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-logging.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/test-logging.c b/tests/test-logging.c
>> index 6387e4933f..8580b82420 100644
>> --- a/tests/test-logging.c
>> +++ b/tests/test-logging.c
>> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>       g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>>   -    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
>> +    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);
>
> Why sometime use this form, ...
>
>>       g_assert(qemu_log_in_addr_range(0));
>>       g_assert(qemu_log_in_addr_range(UINT64_MAX));
>> -
>> +
>>       qemu_set_dfilter_ranges("2..1", &err);
>>       error_free_or_abort(&err);
>
> ... and then this other form?

The first form crashes when the function sets an error.

The second from crashes when the function doesn't set an error, or else
frees the error.

All clear?

Re: [PATCH v2 12/14] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff

[Philippe Mathieu-Daudé]

On 4/22/20 5:19 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Hi Markus,
>>
>> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>>> Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    tests/test-logging.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/test-logging.c b/tests/test-logging.c
>>> index 6387e4933f..8580b82420 100644
>>> --- a/tests/test-logging.c
>>> +++ b/tests/test-logging.c
>>> @@ -73,10 +73,10 @@ static void test_parse_range(void)
>>>        g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>>        g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
>>>    -    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err);
>>> +    qemu_set_dfilter_ranges("0..0xffffffffffffffff", &error_abort);
>>
>> Why sometime use this form, ...
>>
>>>        g_assert(qemu_log_in_addr_range(0));
>>>        g_assert(qemu_log_in_addr_range(UINT64_MAX));
>>> -
>>> +
>>>        qemu_set_dfilter_ranges("2..1", &err);
>>>        error_free_or_abort(&err);
>>
>> ... and then this other form?
> 
> The first form crashes when the function sets an error.
> 
> The second from crashes when the function doesn't set an error, or else
> frees the error.
> 
> All clear?

Yes =) It is even documented!

  * Assert that an expected error occurred, but clean it up without
  * reporting it (primarily useful in testsuites):
  *     error_free_or_abort(&err);

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v2 13/14] qga: Fix qmp_guest_get_memory_blocks() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qmp_guest_get_memory_blocks() passes &local_err to
transfer_memory_block() in a loop.  If this fails in more than one
iteration, it can trip error_setv()'s assertion.

Fix it to break the loop.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a52af0315f..ae1348dc8f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2518,6 +2518,9 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
         mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
         mem_blk->has_can_offline = true; /* lolspeak ftw */
         transfer_memory_block(mem_blk, true, NULL, &local_err);
+        if (local_err) {
+            break;
+        }
 
         entry = g_malloc0(sizeof *entry);
         entry->value = mem_blk;
-- 
2.21.1

Re: [PATCH v2 13/14] qga: Fix qmp_guest_get_memory_blocks() error handling

[Eric Blake]

On 4/22/20 8:07 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qmp_guest_get_memory_blocks() passes &local_err to
> transfer_memory_block() in a loop.  If this fails in more than one
> iteration, it can trip error_setv()'s assertion.
> 
> Fix it to break the loop.
> 
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-posix.c | 3 +++
>   1 file changed, 3 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a52af0315f..ae1348dc8f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2518,6 +2518,9 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>           mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
>           mem_blk->has_can_offline = true; /* lolspeak ftw */
>           transfer_memory_block(mem_blk, true, NULL, &local_err);
> +        if (local_err) {
> +            break;
> +        }
>   
>           entry = g_malloc0(sizeof *entry);
>           entry->value = mem_blk;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second

qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
first to check_suspend_mode(), then to acquire_privilege(), then to
execute_async().  Continuing after errors here can only end in tears.
For instance, we risk tripping error_setv()'s assertion.

Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
Fixes: f54603b6aa765514b2519e74114a2f417759d727
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9717a8d52d..5ba56327dd 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
 
     *mode = GUEST_SUSPEND_MODE_DISK;
     check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
     execute_async(do_suspend, mode, &local_err);
 
+out:
     if (local_err) {
         error_propagate(errp, local_err);
         g_free(mode);
@@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
 
     *mode = GUEST_SUSPEND_MODE_RAM;
     check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
     execute_async(do_suspend, mode, &local_err);
 
+out:
     if (local_err) {
         error_propagate(errp, local_err);
         g_free(mode);
-- 
2.21.1

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Eric Blake]

On 4/22/20 8:07 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> 
> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
> first to check_suspend_mode(), then to acquire_privilege(), then to
> execute_async().  Continuing after errors here can only end in tears.
> For instance, we risk tripping error_setv()'s assertion.
> 
> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
> Fixes: f54603b6aa765514b2519e74114a2f417759d727
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Philippe Mathieu-Daudé]

On 4/22/20 3:07 PM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> 
> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
> first to check_suspend_mode(), then to acquire_privilege(), then to
> execute_async().  Continuing after errors here can only end in tears.
> For instance, we risk tripping error_setv()'s assertion.
> 
> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
> Fixes: f54603b6aa765514b2519e74114a2f417759d727
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 9717a8d52d..5ba56327dd 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_DISK;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {

https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is 
slightly different by removing the if() check.

>           error_propagate(errp, local_err);
>           g_free(mode);
> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_RAM;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {
>           error_propagate(errp, local_err);
>           g_free(mode);
> 

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>>
>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>> first to check_suspend_mode(), then to acquire_privilege(), then to
>> execute_async().  Continuing after errors here can only end in tears.
>> For instance, we risk tripping error_setv()'s assertion.
>>
>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qga/commands-win32.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 9717a8d52d..5ba56327dd 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>         *mode = GUEST_SUSPEND_MODE_DISK;
>>       check_suspend_mode(*mode, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       execute_async(do_suspend, mode, &local_err);
>>   +out:
>>       if (local_err) {
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
> slightly different by removing the if() check.

It frees @mode unconditionally (marked --> below) I believe that's
wrong.  execute_async() runs do_suspend() in a new thread, and passes it
@mode.  do_suspend() frees it.

>>           error_propagate(errp, local_err);
>>           g_free(mode);
>> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>>         *mode = GUEST_SUSPEND_MODE_RAM;
>>       check_suspend_mode(*mode, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       execute_async(do_suspend, mode, &local_err);
>>   +out:
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>           g_free(mode);
>>

   diff --git a/qga/commands-win32.c b/qga/commands-win32.c
   index b49920e201..8b66098056 100644
   --- a/qga/commands-win32.c
   +++ b/qga/commands-win32.c
   @@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)

        *mode = GUEST_SUSPEND_MODE_DISK;
        check_suspend_mode(*mode, &local_err);
   +    if (local_err) {
   +        goto out;
   +    }
        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
   +    if (local_err) {
   +        goto out;
   +    }
        execute_async(do_suspend, mode, &local_err);

   -    if (local_err) {
   -        error_propagate(errp, local_err);
   -        g_free(mode);
   -    }
   +out:
   +    error_propagate(errp, local_err);
-->+    g_free(mode);
    }

    void qmp_guest_suspend_ram(Error **errp)

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Philippe Mathieu-Daudé]

On 4/22/20 5:17 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>> pointer to a variable containing NULL.  Passing an argument of the
>>> latter kind twice without clearing it in between is wrong: if the
>>> first call sets an error, it no longer points to NULL for the second
>>>
>>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>>> first to check_suspend_mode(), then to acquire_privilege(), then to
>>> execute_async().  Continuing after errors here can only end in tears.
>>> For instance, we risk tripping error_setv()'s assertion.
>>>
>>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    qga/commands-win32.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>>> index 9717a8d52d..5ba56327dd 100644
>>> --- a/qga/commands-win32.c
>>> +++ b/qga/commands-win32.c
>>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>>          *mode = GUEST_SUSPEND_MODE_DISK;
>>>        check_suspend_mode(*mode, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        execute_async(do_suspend, mode, &local_err);
>>>    +out:
>>>        if (local_err) {
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
>> slightly different by removing the if() check.
> 
> It frees @mode unconditionally (marked --> below) I believe that's
> wrong.  execute_async() runs do_suspend() in a new thread, and passes it
> @mode.  do_suspend() frees it.

Oops I missed that, good catch!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>>            error_propagate(errp, local_err);
>>>            g_free(mode);
>>> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>>>          *mode = GUEST_SUSPEND_MODE_RAM;
>>>        check_suspend_mode(*mode, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        execute_async(do_suspend, mode, &local_err);
>>>    +out:
>>>        if (local_err) {
>>>            error_propagate(errp, local_err);
>>>            g_free(mode);
>>>
> 
>     diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>     index b49920e201..8b66098056 100644
>     --- a/qga/commands-win32.c
>     +++ b/qga/commands-win32.c
>     @@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)
> 
>          *mode = GUEST_SUSPEND_MODE_DISK;
>          check_suspend_mode(*mode, &local_err);
>     +    if (local_err) {
>     +        goto out;
>     +    }
>          acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>     +    if (local_err) {
>     +        goto out;
>     +    }
>          execute_async(do_suspend, mode, &local_err);
> 
>     -    if (local_err) {
>     -        error_propagate(errp, local_err);
>     -        g_free(mode);
>     -    }
>     +out:
>     +    error_propagate(errp, local_err);
> -->+    g_free(mode);
>      }
> 
>      void qmp_guest_suspend_ram(Error **errp)
> 

Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/22/20 5:17 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 4/22/20 3:07 PM, Markus Armbruster wrote:
>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>> latter kind twice without clearing it in between is wrong: if the
>>>> first call sets an error, it no longer points to NULL for the second
>>>>
>>>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>>>> first to check_suspend_mode(), then to acquire_privilege(), then to
>>>> execute_async().  Continuing after errors here can only end in tears.
>>>> For instance, we risk tripping error_setv()'s assertion.
>>>>
>>>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>>>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>>>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    qga/commands-win32.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>>>> index 9717a8d52d..5ba56327dd 100644
>>>> --- a/qga/commands-win32.c
>>>> +++ b/qga/commands-win32.c
>>>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>>>          *mode = GUEST_SUSPEND_MODE_DISK;
>>>>        check_suspend_mode(*mode, &local_err);
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>>        execute_async(do_suspend, mode, &local_err);
>>>>    +out:
>>>>        if (local_err) {
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
>>> slightly different by removing the if() check.
>>
>> It frees @mode unconditionally (marked --> below) I believe that's
>> wrong.  execute_async() runs do_suspend() in a new thread, and passes it
>> @mode.  do_suspend() frees it.
>
> Oops I missed that, good catch!
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

I wasn't aware of (or totally forgot about) your patch, or else I'd have
fixed it instead of redoing it.  My apologies!

Re: [PATCH v2 00/14] Miscellaneous error handling fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200422130719.28225-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v2 00/14] Miscellaneous error handling fixes
Message-id: 20200422130719.28225-1-armbru@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
dd69af1 qga: Fix qmp_guest_suspend_{disk, ram}() error handling
4a08f31 qga: Fix qmp_guest_get_memory_blocks() error handling
637065b tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff
373e741 migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
aee8384 io: Fix qio_channel_socket_close() error handling
d6149e9 xen/pt: Fix flawed conversion to realize()
698866e virtio-net: Fix duplex=... and speed=... error handling
ead7d5b bochs-display: Fix vgamem=SIZE error handling
e36a75a fdc: Fix fallback=auto error handling
3840993 arm/virt: Fix virt_machine_device_plug_cb() error API violation
e673dde cpus: Proper range-checking for -icount shift=N
ec03bd5 cpus: Fix configure_icount() error API violation
b7227e5 block/file-posix: Fix check_cache_dropped() error handling
c2d6383 cryptodev: Fix cryptodev_builtin_cleanup() error API violation

=== OUTPUT BEGIN ===
1/14 Checking commit c2d63831898d (cryptodev: Fix cryptodev_builtin_cleanup() error API violation)
2/14 Checking commit b7227e54d3e2 (block/file-posix: Fix check_cache_dropped() error handling)
3/14 Checking commit ec03bd56477a (cpus: Fix configure_icount() error API violation)
ERROR: consider using qemu_strtol in preference to strtol
#58: FILE: cpus.c:818:
+        time_shift = strtol(option, &rem_str, 0);

total: 1 errors, 0 warnings, 70 lines checked

Patch 3/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/14 Checking commit e673dde157bc (cpus: Proper range-checking for -icount shift=N)
5/14 Checking commit 384099345bd0 (arm/virt: Fix virt_machine_device_plug_cb() error API violation)
6/14 Checking commit e36a75a34539 (fdc: Fix fallback=auto error handling)
7/14 Checking commit ead7d5bb1f48 (bochs-display: Fix vgamem=SIZE error handling)
8/14 Checking commit 698866e34497 (virtio-net: Fix duplex=... and speed=... error handling)
9/14 Checking commit d6149e9c741d (xen/pt: Fix flawed conversion to realize())
10/14 Checking commit aee83842a8dc (io: Fix qio_channel_socket_close() error handling)
11/14 Checking commit 373e741c45db (migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling)
12/14 Checking commit 637065bdb8fe (tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff)
13/14 Checking commit 4a08f313759b (qga: Fix qmp_guest_get_memory_blocks() error handling)
14/14 Checking commit dd69af1d8010 (qga: Fix qmp_guest_suspend_{disk, ram}() error handling)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200422130719.28225-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 00/14] Miscellaneous error handling fixes

[Markus Armbruster]

Queued.