[Qemu-devel] [PATCH] block: Don't copy backing file name on error

[Qemu-devel] [PATCH] block: Don't copy backing file name on error

[Max Reitz]

bdrv_open_backing_file() tries to copy the backing file name using
pstrcpy directly after calling bdrv_open() to open the backing file
without checking whether that was actually successful. If it was not,
ps->backing_hd->file will probably be NULL and qemu will crash.

Fix this by moving pstrcpy after checking whether bdrv_open() succeeded.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 4474012..61795fe 100644
--- a/block.c
+++ b/block.c
@@ -1005,8 +1005,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     ret = bdrv_open(bs->backing_hd,
                     *backing_filename ? backing_filename : NULL, options,
                     back_flags, back_drv, &local_err);
-    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-            bs->backing_hd->file->filename);
     if (ret < 0) {
         bdrv_unref(bs->backing_hd);
         bs->backing_hd = NULL;
@@ -1014,6 +1012,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         error_propagate(errp, local_err);
         return ret;
     }
+    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+            bs->backing_hd->file->filename);
     return 0;
 }
 
-- 
1.8.4.1

Re: [Qemu-devel] [PATCH] block: Don't copy backing file name on error

[Amos Kong]

On Sat, Oct 26, 2013 at 9:44 PM, Max Reitz <mreitz@redhat.com> wrote:
>
> bdrv_open_backing_file() tries to copy the backing file name using
> pstrcpy directly after calling bdrv_open() to open the backing file
> without checking whether that was actually successful. If it was not,
> ps->backing_hd->file will probably be NULL and qemu will crash.
>
> Fix this by moving pstrcpy after checking whether bdrv_open() succeeded.

Reviewed-by: Amos Kong <kongjianjun@gmail.com>

>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 4474012..61795fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -1005,8 +1005,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      ret = bdrv_open(bs->backing_hd,
>                      *backing_filename ? backing_filename : NULL, options,
>                      back_flags, back_drv, &local_err);
> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> -            bs->backing_hd->file->filename);
>      if (ret < 0) {
>          bdrv_unref(bs->backing_hd);
>          bs->backing_hd = NULL;
> @@ -1014,6 +1012,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          error_propagate(errp, local_err);
>          return ret;
>      }
> +    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +            bs->backing_hd->file->filename);
>      return 0;
>  }
>
> --
> 1.8.4.1
>
>

Re: [Qemu-devel] [PATCH] block: Don't copy backing file name on error

[Benoît Canet]

Le Saturday 26 Oct 2013 à 15:44:43 (+0200), Max Reitz a écrit :
> bdrv_open_backing_file() tries to copy the backing file name using
> pstrcpy directly after calling bdrv_open() to open the backing file
> without checking whether that was actually successful. If it was not,
> ps->backing_hd->file will probably be NULL and qemu will crash.
> 
> Fix this by moving pstrcpy after checking whether bdrv_open() succeeded.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4474012..61795fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -1005,8 +1005,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      ret = bdrv_open(bs->backing_hd,
>                      *backing_filename ? backing_filename : NULL, options,
>                      back_flags, back_drv, &local_err);
> -    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> -            bs->backing_hd->file->filename);
>      if (ret < 0) {
>          bdrv_unref(bs->backing_hd);
>          bs->backing_hd = NULL;
> @@ -1014,6 +1012,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          error_propagate(errp, local_err);
>          return ret;
>      }
> +    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +            bs->backing_hd->file->filename);
>      return 0;
>  }
>  
> -- 
> 1.8.4.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH] block: Don't copy backing file name on error

[Kevin Wolf]

Am 26.10.2013 um 15:44 hat Max Reitz geschrieben:
> bdrv_open_backing_file() tries to copy the backing file name using
> pstrcpy directly after calling bdrv_open() to open the backing file
> without checking whether that was actually successful. If it was not,
> ps->backing_hd->file will probably be NULL and qemu will crash.
> 
> Fix this by moving pstrcpy after checking whether bdrv_open() succeeded.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Thanks, applied. A test case wouldn't hurt, though.

Kevin