[PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[Max Reitz]

Hi,

When reviewing David’s series to add --target-is-zero convert, I looked
for a case to show that the current implementation will crash if
-n --target-is-zero is used together with -B.  It then turned out that
-B will always crash when combined with -n and the target image does not
have a backing file set in its image header.

This series fixes that.


Max Reitz (2):
  qemu-img: Fix convert -n -B for backing-less targets
  iotests: Test convert -n -B to backing-less target

 qemu-img.c                 |  2 +-
 tests/qemu-iotests/122     | 14 ++++++++++++++
 tests/qemu-iotests/122.out |  5 +++++
 3 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.24.1

[PATCH 1/2] qemu-img: Fix convert -n -B for backing-less targets

[Max Reitz]

s.target_has_backing does not reflect whether the target BDS has a
backing file; it only tells whether we should use a backing file during
conversion (specified by -B).

As such, if you use convert -n, the target does not necessarily actually
have a backing file, and then dereferencing out_bs->backing fails here.

When converting to an existing file, we should set
target_backing_sectors to a negative value, because first, as the
comment explains, this value is only used for optimization, so it is
always fine to do that.

Second, we use this value to determine where the target must be
initialized to zeroes (overlays are initialized to zero after the end of
their backing file).  When converting to an existing file, we cannot
assume that to be true.

Cc: qemu-stable@nongnu.org
Fixes: 351c8efff9ad809c822d55620df54d575d536f68
       ("qemu-img: Special post-backing convert handling")
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..89a1d11781 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2503,7 +2503,7 @@ static int img_convert(int argc, char **argv)
         }
     }
 
-    if (s.target_has_backing) {
+    if (s.target_has_backing && s.target_is_new) {
         /* Errors are treated as "backing length unknown" (which means
          * s.target_backing_sectors has to be negative, which it will
          * be automatically).  The backing file length is used only
-- 
2.24.1

[PATCH 2/2] iotests: Test convert -n -B to backing-less target

[Max Reitz]

This must not crash.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/122     | 14 ++++++++++++++
 tests/qemu-iotests/122.out |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index dfa350936f..f7a3ae684a 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -276,6 +276,20 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
 
 $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
 
+echo
+echo '=== -n -B to an image without a backing file ==='
+echo
+
+# Base for the output
+TEST_IMG="$TEST_IMG".base _make_test_img 64M
+
+# Output that does have $TEST_IMG.base set as its (implicit) backing file
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+
+# Convert with -n, which should not confuse -B with "target BDS has a
+# backing file"
+$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -n "$TEST_IMG" "$TEST_IMG".orig
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 849b6cc2ef..1a35951a80 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -228,4 +228,9 @@ Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Images are identical.
+
+=== -n -B to an image without a backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
 *** done
-- 
2.24.1

Re: [PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[John Snow]

On 1/21/20 10:59 AM, Max Reitz wrote:
> Hi,
> 
> When reviewing David’s series to add --target-is-zero convert, I looked
> for a case to show that the current implementation will crash if
> -n --target-is-zero is used together with -B.  It then turned out that
> -B will always crash when combined with -n and the target image does not
> have a backing file set in its image header.
> 
> This series fixes that.
> 
> 
> Max Reitz (2):
>   qemu-img: Fix convert -n -B for backing-less targets
>   iotests: Test convert -n -B to backing-less target
> 
>  qemu-img.c                 |  2 +-
>  tests/qemu-iotests/122     | 14 ++++++++++++++
>  tests/qemu-iotests/122.out |  5 +++++
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 

Hello.
Makes sense to me.

Reviewed-by: John Snow <jsnow@redhat.com>

(My brain had an awfully tumultuous 35 seconds comprehending that
"is_new" was not a synonym for "-n was provided", but actually means the
opposite.)

Re: [PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]

On 21.01.20 16:59, Max Reitz wrote:
> Hi,
> 
> When reviewing David’s series to add --target-is-zero convert, I looked
> for a case to show that the current implementation will crash if
> -n --target-is-zero is used together with -B.  It then turned out that
> -B will always crash when combined with -n and the target image does not
> have a backing file set in its image header.
> 
> This series fixes that.

Thanks for the review, applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]