Patchwork [v2,1/2] block/drive-mirror: Check for NULL backing_hd

login
register
mail settings
Submitter Max Reitz
Date Nov. 5, 2013, 12:35 a.m.
Message ID <1383611706-22107-2-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/288350/
State New
Headers show

Comments

Max Reitz - Nov. 5, 2013, 12:35 a.m.
It should be possible to execute the QMP "drive-mirror" command in
"none" sync mode and "absolute-paths" mode even for block devices
lacking a backing file.

"absolute-paths" does in fact not require a backing file to be present,
as can be seen from the "top" sync mode code path. "top" basically
states that the device should indeed have a backing file - however, the
current code catches the case if it doesn't and then simply treats it as
"full" sync mode, creating a target image without a backing file (in
"absolute-paths" mode). Thus, "absolute-paths" does not imply the target
file must indeed have a backing file.

Therefore, the target file may be left unbacked in case of "none" sync
mode as well, if the specified device is not backed either. Currently,
qemu will crash trying to dereference the backing file pointer since it
assumes that it will always be non-NULL in that case ("none" with
"absolute-paths").

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Fam Zheng - Nov. 5, 2013, 3:32 a.m.
On 11/05/2013 08:35 AM, Max Reitz wrote:
> It should be possible to execute the QMP "drive-mirror" command in
> "none" sync mode and "absolute-paths" mode even for block devices
> lacking a backing file.
>
> "absolute-paths" does in fact not require a backing file to be present,
> as can be seen from the "top" sync mode code path. "top" basically
> states that the device should indeed have a backing file - however, the
> current code catches the case if it doesn't and then simply treats it as
> "full" sync mode, creating a target image without a backing file (in
> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
> file must indeed have a backing file.
>
> Therefore, the target file may be left unbacked in case of "none" sync
> mode as well, if the specified device is not backed either. Currently,
> qemu will crash trying to dereference the backing file pointer since it
> assumes that it will always be non-NULL in that case ("none" with
> "absolute-paths").
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   blockdev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b260477..986e59d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>           case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>               /* create new image with backing file */
>               bdrv_img_create(target, format,
> -                            source->filename,
> -                            source->drv->format_name,
> +                            source ? source->filename : NULL,
> +                            source ? source->drv->format_name : NULL,
>                               NULL, size, flags, &local_err, false);
>               break;
>           default:
The code around here is:

2029     if (sync == MIRROR_SYNC_MODE_FULL && mode != 
NEW_IMAGE_MODE_EXISTING) {
2030         /* create new image w/o backing file */
2031         assert(format && drv);
2032         bdrv_img_create(target, format,
2033                         NULL, NULL, NULL, size, flags, &local_err, 
false);
2034     } else {
2035         switch (mode) {
2036         case NEW_IMAGE_MODE_EXISTING:
2037             break;
2038         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
2039             /* create new image with backing file */
2040             bdrv_img_create(target, format,
2041                             source->filename,
2042                             source->drv->format_name,
2043                             NULL, size, flags, &local_err, false);
2044             break;
2045         default:
2046             abort();
2047         }
2048     }

Why not update the if condition and reuse the branch, I think this is a 
better branching? Either should be fine, but in your change you should 
also update the comment in line 2039.

Thanks,
Fam
Max Reitz - Nov. 5, 2013, 7:06 p.m.
On 05.11.2013 04:32, Fam Zheng wrote:
>
> On 11/05/2013 08:35 AM, Max Reitz wrote:
>> It should be possible to execute the QMP "drive-mirror" command in
>> "none" sync mode and "absolute-paths" mode even for block devices
>> lacking a backing file.
>>
>> "absolute-paths" does in fact not require a backing file to be present,
>> as can be seen from the "top" sync mode code path. "top" basically
>> states that the device should indeed have a backing file - however, the
>> current code catches the case if it doesn't and then simply treats it as
>> "full" sync mode, creating a target image without a backing file (in
>> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
>> file must indeed have a backing file.
>>
>> Therefore, the target file may be left unbacked in case of "none" sync
>> mode as well, if the specified device is not backed either. Currently,
>> qemu will crash trying to dereference the backing file pointer since it
>> assumes that it will always be non-NULL in that case ("none" with
>> "absolute-paths").
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   blockdev.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b260477..986e59d 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const
>> char *target,
>>           case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>>               /* create new image with backing file */
>>               bdrv_img_create(target, format,
>> -                            source->filename,
>> -                            source->drv->format_name,
>> +                            source ? source->filename : NULL,
>> +                            source ? source->drv->format_name : NULL,
>>                               NULL, size, flags, &local_err, false);
>>               break;
>>           default:
> The code around here is:
>
> 2029     if (sync == MIRROR_SYNC_MODE_FULL && mode !=
> NEW_IMAGE_MODE_EXISTING) {
> 2030         /* create new image w/o backing file */
> 2031         assert(format && drv);
> 2032         bdrv_img_create(target, format,
> 2033                         NULL, NULL, NULL, size, flags,
> &local_err, false);
> 2034     } else {
> 2035         switch (mode) {
> 2036         case NEW_IMAGE_MODE_EXISTING:
> 2037             break;
> 2038         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
> 2039             /* create new image with backing file */
> 2040             bdrv_img_create(target, format,
> 2041                             source->filename,
> 2042                             source->drv->format_name,
> 2043                             NULL, size, flags, &local_err, false);
> 2044             break;
> 2045         default:
> 2046             abort();
> 2047         }
> 2048     }
>
> Why not update the if condition and reuse the branch, I think this is
> a better branching? Either should be fine, but in your change you
> should also update the comment in line 2039.

Okay, I'll go for updating the condition.

Max

> Thanks,
> Fam

Patch

diff --git a/blockdev.c b/blockdev.c
index b260477..986e59d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2038,8 +2038,8 @@  void qmp_drive_mirror(const char *device, const char *target,
         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
             /* create new image with backing file */
             bdrv_img_create(target, format,
-                            source->filename,
-                            source->drv->format_name,
+                            source ? source->filename : NULL,
+                            source ? source->drv->format_name : NULL,
                             NULL, size, flags, &local_err, false);
             break;
         default: