[PULL 0/6] Block patches for 5.0-rc0

[PULL 0/6] Block patches for 5.0-rc0

[Max Reitz]

The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2020-03-23 20:54:24 +0000)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24

for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:

  iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)

----------------------------------------------------------------
Block patches for 5.0-rc0:
- Use-after-free fix
- Fix for a memleak in an error path
- Preventative measures against other potential use-after-frees, and
  against NULL deferences at runtime
- iotest fixes

----------------------------------------------------------------
Eric Blake (1):
  block: Avoid memleak on qcow2 image info failure

Max Reitz (2):
  iotests: Fix cleanup path in some tests
  iotests/026: Move v3-exclusive test to new file

Philippe Mathieu-Daudé (1):
  block: Assert BlockDriver::format_name is not NULL

Vladimir Sementsov-Ogievskiy (2):
  block: bdrv_set_backing_bs: fix use-after-free
  block/qcow2: zero data_file child after free

 block.c                            |  3 +-
 block/qcow2.c                      |  3 +
 tests/qemu-iotests/026             | 31 -----------
 tests/qemu-iotests/026.out         |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/085             |  1 +
 tests/qemu-iotests/087             |  6 ++
 tests/qemu-iotests/279             |  2 +-
 tests/qemu-iotests/289             | 89 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/289.out         |  8 +++
 tests/qemu-iotests/group           |  1 +
 11 files changed, 111 insertions(+), 45 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

-- 
2.25.1

[PULL 1/6] block: Avoid memleak on qcow2 image info failure

[Max Reitz]

From: Eric Blake <eblake@redhat.com>

If we fail to get bitmap info, we must not leak the encryption info.

Fixes: b8968c875f403
Fixes: Coverity CID 1421894
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200320183620.1112123-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..e08917ed84 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4811,6 +4811,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
         if (local_err) {
             error_propagate(errp, local_err);
             qapi_free_ImageInfoSpecific(spec_info);
+            qapi_free_QCryptoBlockInfo(encrypt_info);
             return NULL;
         }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
-- 
2.25.1

[PULL 2/6] block: Assert BlockDriver::format_name is not NULL

[Max Reitz]

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

bdrv_do_find_format() calls strcmp() using BlockDriver::format_name
as argument, which must not be NULL. Assert this field is not null
when we register a block driver in bdrv_register().

Reported-by: Mansour Ahmadi <ManSoSec@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200318222235.23856-1-philmd@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index a2542c977b..6b984dc883 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
 
 void bdrv_register(BlockDriver *bdrv)
 {
+    assert(bdrv->format_name);
     QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
 }
 
-- 
2.25.1

[PULL 3/6] block: bdrv_set_backing_bs: fix use-after-free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

There is a use-after-free possible: bdrv_unref_child() leaves
bs->backing freed but not NULL. bdrv_attach_child may produce nested
polling loop due to drain, than access of freed pointer is possible.

I've produced the following crash on 30 iotest with modified code. It
does not reproduce on master, but still seems possible:

    #0  __strcmp_avx2 () at /lib64/libc.so.6
    #1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
    #2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
    #3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
    #4  bdrv_replace_child_noperm
        (child=child@entry=0x55c9d48e5520,
        new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
    #5  bdrv_replace_child
        (child=child@entry=0x55c9d48e5520,
        new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
    #6  bdrv_root_attach_child
        (child_bs=child_bs@entry=0x55c9d3cc2060,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
        opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
    #7  bdrv_attach_child
        (parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
        child_bs=child_bs@entry=0x55c9d3cc2060,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        errp=errp@entry=0x7ffd117108e0) at block.c:5876
    #8  in bdrv_set_backing_hd
        (bs=bs@entry=0x55c9d3c5a3d0,
        backing_hd=backing_hd@entry=0x55c9d3cc2060,
        errp=errp@entry=0x7ffd117108e0)
        at block.c:2576
    #9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
    #10 job_prepare (job=0x55c9d49d84a0) at job.c:761
    #11 job_txn_apply (txn=<optimized out>, fn=<optimized out>) at
        job.c:145
    #12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
    #13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
    #14 job_completed (job=0x55c9d49d84a0) at job.c:845
    #15 job_completed (job=0x55c9d49d84a0) at job.c:836
    #16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
    #17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
    #18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
    #19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
        blocking=blocking@entry=true)
        at util/aio-posix.c:728
    #20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
        at block/io.c:121
    #21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
        poll=poll@entry=true)
        at block/io.c:114
    #22 bdrv_replace_child_noperm
        (child=child@entry=0x55c9d3d558f0,
        new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
    #23 bdrv_replace_child
        (child=child@entry=0x55c9d3d558f0,
        new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
    #24 bdrv_root_attach_child
        (child_bs=child_bs@entry=0x55c9d3d27300,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
        opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
    #25 bdrv_attach_child
        (parent_bs=parent_bs@entry=0x55c9d3cc2060,
        child_bs=child_bs@entry=0x55c9d3d27300,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        errp=errp@entry=0x7ffd11710c60) at block.c:5876
    #26 bdrv_set_backing_hd
        (bs=bs@entry=0x55c9d3cc2060,
        backing_hd=backing_hd@entry=0x55c9d3d27300,
        errp=errp@entry=0x7ffd11710c60)
        at block.c:2576
    #27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
    ...

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20200316060631.30052-2-vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 6b984dc883..cccae5add9 100644
--- a/block.c
+++ b/block.c
@@ -2760,10 +2760,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
 
     if (bs->backing) {
         bdrv_unref_child(bs, bs->backing);
+        bs->backing = NULL;
     }
 
     if (!backing_hd) {
-        bs->backing = NULL;
         goto out;
     }
 
-- 
2.25.1

[PULL 4/6] block/qcow2: zero data_file child after free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

data_file being NULL doesn't seem to be a correct state, but it's
better than dead pointer and simpler to debug.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20200316060631.30052-3-vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index e08917ed84..d1da3d91db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     g_free(s->image_data_file);
     if (has_data_file(bs)) {
         bdrv_unref_child(bs, s->data_file);
+        s->data_file = NULL;
     }
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
@@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
 
     if (has_data_file(bs)) {
         bdrv_unref_child(bs, s->data_file);
+        s->data_file = NULL;
     }
 
     qcow2_refcount_close(bs);
-- 
2.25.1

[PULL 5/6] iotests: Fix cleanup path in some tests

[Max Reitz]

Some iotests leave behind some external data file when run for qcow2
with -o data_file.  Fix that.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200224171631.384314-1-mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/085 | 1 +
 tests/qemu-iotests/087 | 6 ++++++
 tests/qemu-iotests/279 | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 46981dbb64..dd3c993a2d 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -39,6 +39,7 @@ SNAPSHOTS=10
 _cleanup()
 {
     _cleanup_qemu
+    _cleanup_test_img
     for i in $(seq 1 ${SNAPSHOTS})
     do
         _rm_test_img "${TEST_DIR}/${i}-${snapshot_virt0}"
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index d6c8613419..bdfdad3454 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -26,6 +26,12 @@ echo "QA output created by $seq"
 
 status=1	# failure is the default!
 
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/279 b/tests/qemu-iotests/279
index 30d29b1cb2..75a4747e6b 100755
--- a/tests/qemu-iotests/279
+++ b/tests/qemu-iotests/279
@@ -26,7 +26,7 @@ status=1	# failure is the default!
 _cleanup()
 {
     _cleanup_test_img
-    rm -f "$TEST_IMG.mid"
+    _rm_test_img "$TEST_IMG.mid"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
-- 
2.25.1

[PULL 6/6] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

data_file does not work with v2, and we probably want 026 to keep
working for v2 images.  Thus, open a new file for v3-exclusive error
path test cases.

Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
       (“iotests/026: Test EIO on allocation in a data-file”)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200311140707.1243218-1-mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Tested-by: John Snow <jsnow@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/026             | 31 -----------
 tests/qemu-iotests/026.out         |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/289             | 89 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/289.out         |  8 +++
 tests/qemu-iotests/group           |  1 +
 6 files changed, 98 insertions(+), 43 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index b05a4692cf..b9713eb591 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -240,37 +240,6 @@ $QEMU_IO -c "write 0 $CLUSTER_SIZE" "$BLKDBG_TEST_IMG" | _filter_qemu_io
 
 _check_test_img
 
-echo
-echo === Avoid freeing external data clusters on failure ===
-echo
-
-# Similar test as the last one, except we test what happens when there
-# is an error when writing to an external data file instead of when
-# writing to a preallocated zero cluster
-_make_test_img -o "data_file=$TEST_IMG.data_file" $CLUSTER_SIZE
-
-# Put blkdebug above the data-file, and a raw node on top of that so
-# that blkdebug will see a write_aio event and emit an error
-$QEMU_IO -c "write 0 $CLUSTER_SIZE" \
-    "json:{
-         'driver': 'qcow2',
-         'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
-         'data-file': {
-             'driver': 'raw',
-             'file': {
-                 'driver': 'blkdebug',
-                 'config': '$TEST_DIR/blkdebug.conf',
-                 'image': {
-                     'driver': 'file',
-                     'filename': '$TEST_IMG.data_file'
-                 }
-             }
-         }
-     }" \
-    | _filter_qemu_io
-
-_check_test_img
-
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index c1b3b58482..83989996ff 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -653,10 +653,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/026.out.nocache b/tests/qemu-iotests/026.out.nocache
index 8d5001648a..9359d26d7e 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -661,10 +661,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 0000000000..1c11d4030e
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,89 @@
+#!/usr/bin/env bash
+#
+# qcow2 v3-exclusive error path testing
+# (026 tests paths common to v2 and v3)
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm "$TEST_DIR/blkdebug.conf"
+    rm -f "$TEST_IMG.data_file"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt qcow2
+_supported_proto file
+# This is a v3-exclusive test;
+# As for data_file, error paths often very much depend on whether
+# there is an external data file or not; so we create one exactly when
+# we want to test it
+_unsupported_imgopts 'compat=0.10' data_file
+
+echo
+echo === Avoid freeing external data clusters on failure ===
+echo
+
+cat > "$TEST_DIR/blkdebug.conf" <<EOF
+[inject-error]
+event = "write_aio"
+errno = "5"
+once = "on"
+EOF
+
+# Test what happens when there is an error when writing to an external
+# data file instead of when writing to a preallocated zero cluster
+_make_test_img -o "data_file=$TEST_IMG.data_file" 64k
+
+# Put blkdebug above the data-file, and a raw node on top of that so
+# that blkdebug will see a write_aio event and emit an error.  This
+# will then trigger the alloc abort code, which we want to test here.
+$QEMU_IO -c "write 0 64k" \
+    "json:{
+         'driver': 'qcow2',
+         'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
+         'data-file': {
+             'driver': 'raw',
+             'file': {
+                 'driver': 'blkdebug',
+                 'config': '$TEST_DIR/blkdebug.conf',
+                 'image': {
+                     'driver': 'file',
+                     'filename': '$TEST_IMG.data_file'
+                 }
+             }
+         }
+     }" \
+    | _filter_qemu_io
+
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/289.out b/tests/qemu-iotests/289.out
new file mode 100644
index 0000000000..e54e2629d4
--- /dev/null
+++ b/tests/qemu-iotests/289.out
@@ -0,0 +1,8 @@
+QA output created by 289
+
+=== Avoid freeing external data clusters on failure ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 data_file=TEST_DIR/t.IMGFMT.data_file
+write failed: Input/output error
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ec2b2302e5..79c6dfc85d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -295,3 +295,4 @@
 284 rw
 286 rw quick
 288 quick
+289 rw quick
-- 
2.25.1

Re: [PULL 0/6] Block patches for 5.0-rc0

[Peter Maydell]

On Tue, 24 Mar 2020 at 12:21, Max Reitz <mreitz@redhat.com> wrote:
>
> The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2020-03-23 20:54:24 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24
>
> for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:
>
>   iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)
>
> ----------------------------------------------------------------
> Block patches for 5.0-rc0:
> - Use-after-free fix
> - Fix for a memleak in an error path
> - Preventative measures against other potential use-after-frees, and
>   against NULL deferences at runtime
> - iotest fixes

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM