[PATCH] block/snapshot: fix *errp handling in bdrv_snapshot_goto

[PATCH] block/snapshot: fix *errp handling in bdrv_snapshot_goto

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This error was discovered by fuzzing qemu-img.

If bdrv_snapshot_goto() returns an error, it is not handled immediately,
allowing *errp to be reassigned when qcow_open() fails, which triggers
assert(*errp == NULL) in util/error.c: void error_setv().

This patch ensures that errors from bdrv_snapshot_goto() are handled
immediately after the call, preventing *errp from being modified twice
and avoiding unnecessary assertion failures.

Closes: https://gitlab.com/qemu-project/qemu/-/issues/2851
Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 block/snapshot.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9c44780e96..d1b5a8d33d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -296,14 +296,20 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         bdrv_graph_wrunlock();
 
         ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
+        if (ret < 0) {
+            bdrv_unref(fallback_bs);
+            bs->drv = NULL;
+            /* A bdrv_snapshot_goto() error takes precedence */
+            error_propagate(errp, local_err);
+            return ret;
+        }
         open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
         qobject_unref(options);
         if (open_ret < 0) {
             bdrv_unref(fallback_bs);
             bs->drv = NULL;
-            /* A bdrv_snapshot_goto() error takes precedence */
             error_propagate(errp, local_err);
-            return ret < 0 ? ret : open_ret;
+            return open_ret;
         }
 
         /*
-- 
2.42.2

Re: [PATCH] block/snapshot: fix *errp handling in bdrv_snapshot_goto

[Kevin Wolf]

Am 04.03.2025 um 09:01 hat gerben@altlinux.org geschrieben:
> From: Denis Rastyogin <gerben@altlinux.org>
> 
> This error was discovered by fuzzing qemu-img.
> 
> If bdrv_snapshot_goto() returns an error, it is not handled immediately,
> allowing *errp to be reassigned when qcow_open() fails, which triggers
> assert(*errp == NULL) in util/error.c: void error_setv().
> 
> This patch ensures that errors from bdrv_snapshot_goto() are handled
> immediately after the call, preventing *errp from being modified twice
> and avoiding unnecessary assertion failures.
> 
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/2851
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
> ---
>  block/snapshot.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9c44780e96..d1b5a8d33d 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -296,14 +296,20 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>          bdrv_graph_wrunlock();
>  
>          ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
> +        if (ret < 0) {
> +            bdrv_unref(fallback_bs);
> +            bs->drv = NULL;
> +            /* A bdrv_snapshot_goto() error takes precedence */
> +            error_propagate(errp, local_err);
> +            return ret;
> +        }

Now you return without having reopened the image!

>          open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
>          qobject_unref(options);
>          if (open_ret < 0) {
>              bdrv_unref(fallback_bs);
>              bs->drv = NULL;
> -            /* A bdrv_snapshot_goto() error takes precedence */
>              error_propagate(errp, local_err);
> -            return ret < 0 ? ret : open_ret;
> +            return open_ret;
>          }

I don't see a problem in the old code. local_err is still NULL at this
point, so it's safe to pass it to drv->bdrv_open(). A previous error
from bdrv_snapshot_goto() is stored in errp, which is separate.

The line in qcow.c (test.qed is misleading as a filename!) that causes
the assertion failure is this:

    error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
               "does not support live migration",
               bdrv_get_device_or_node_name(bs));

This is an internal field that is not related to either error in
bdrv_snapshot_goto(). This is another instance of the same bug that
bs->opaque needs to be zeroed before calling drv->bdrv_open(). I sent a
patch for that.

Kevin