* [Qemu-devel] [PATCH] block: qcow2: free 'refcount_table' in error path
@ 2019-08-31 2:04 Li Qiang
2019-09-03 10:22 ` Kevin Wolf
0 siblings, 1 reply; 2+ messages in thread
From: Li Qiang @ 2019-08-31 2:04 UTC (permalink / raw)
To: kwolf, mreitz; +Cc: Li Qiang, liq3ea, qemu-devel, qemu-block
Currently, when doing './check -qcow2 098'. We can get following
asan output:
qemu-img: Could not empty blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
+
+=================================================================
+==60365==ERROR: LeakSanitizer: detected memory leaks
+
+Direct leak of 65536 byte(s) in 1 object(s) allocated from:
+ #0 0x7f3ed729fd38 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded38)
+ #1 0x56274517fe66 in make_completely_empty block/IMGFMT.c:4219
+ #2 0x562745180e51 in IMGFMT_make_empty block/IMGFMT.c:4313
+ #3 0x56274509b14e in img_commit /home/test/qemu5/qemu/qemu-img.c:1053
+ #4 0x5627450b4b74 in main /home/test/qemu5/qemu/qemu-img.c:5097
+ #5 0x7f3ed4f2fb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
This is because the logic of clean resource in 'make_completely_empty' is
wrong. The patch frees the 's->refcount_table' in error path.
Signed-off-by: Li Qiang <liq3ea@163.com>
---
block/qcow2.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c5a4859f7..23fe713d4c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4243,7 +4243,7 @@ static int make_completely_empty(BlockDriverState *bs)
ret = bdrv_pwrite_sync(bs->file, s->cluster_size,
&rt_entry, sizeof(rt_entry));
if (ret < 0) {
- goto fail_broken_refcounts;
+ goto fail;
}
s->refcount_table[0] = 2 * s->cluster_size;
@@ -4252,7 +4252,7 @@ static int make_completely_empty(BlockDriverState *bs)
offset = qcow2_alloc_clusters(bs, 3 * s->cluster_size + l1_size2);
if (offset < 0) {
ret = offset;
- goto fail_broken_refcounts;
+ goto fail;
} else if (offset > 0) {
error_report("First cluster in emptied image is in use");
abort();
@@ -4274,6 +4274,9 @@ static int make_completely_empty(BlockDriverState *bs)
return 0;
+fail:
+ g_free(s->refcount_table);
+
fail_broken_refcounts:
/* The BDS is unusable at this point. If we wanted to make it usable, we
* would have to call qcow2_refcount_close(), qcow2_refcount_init(),
@@ -4283,8 +4286,6 @@ fail_broken_refcounts:
* that that sequence will fail as well. Therefore, just eject the BDS. */
bs->drv = NULL;
-fail:
- g_free(new_reftable);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] block: qcow2: free 'refcount_table' in error path
2019-08-31 2:04 [Qemu-devel] [PATCH] block: qcow2: free 'refcount_table' in error path Li Qiang
@ 2019-09-03 10:22 ` Kevin Wolf
0 siblings, 0 replies; 2+ messages in thread
From: Kevin Wolf @ 2019-09-03 10:22 UTC (permalink / raw)
To: Li Qiang; +Cc: liq3ea, qemu-devel, qemu-block, mreitz
Am 31.08.2019 um 04:04 hat Li Qiang geschrieben:
> Currently, when doing './check -qcow2 098'. We can get following
> asan output:
>
> qemu-img: Could not empty blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
> +
> +=================================================================
> +==60365==ERROR: LeakSanitizer: detected memory leaks
> +
> +Direct leak of 65536 byte(s) in 1 object(s) allocated from:
> + #0 0x7f3ed729fd38 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded38)
> + #1 0x56274517fe66 in make_completely_empty block/IMGFMT.c:4219
> + #2 0x562745180e51 in IMGFMT_make_empty block/IMGFMT.c:4313
> + #3 0x56274509b14e in img_commit /home/test/qemu5/qemu/qemu-img.c:1053
> + #4 0x5627450b4b74 in main /home/test/qemu5/qemu/qemu-img.c:5097
> + #5 0x7f3ed4f2fb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
>
> This is because the logic of clean resource in 'make_completely_empty' is
> wrong. The patch frees the 's->refcount_table' in error path.
>
> Signed-off-by: Li Qiang <liq3ea@163.com>
This is wrong. You can never free s->refcount_table and leave it as a
dangling pointer. It is state that is only supposed to be freed in
qcow2_close() -> qcow2_refcount_close().
The only reason why it doesn't crash with your change is that you also
make the error fatal (bs->drv = NULL) so that any further I/O on the
image will fail anyway. But there is no good reason to make these errors
fatal.
Kevin
> block/qcow2.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7c5a4859f7..23fe713d4c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4243,7 +4243,7 @@ static int make_completely_empty(BlockDriverState *bs)
> ret = bdrv_pwrite_sync(bs->file, s->cluster_size,
> &rt_entry, sizeof(rt_entry));
> if (ret < 0) {
> - goto fail_broken_refcounts;
> + goto fail;
> }
> s->refcount_table[0] = 2 * s->cluster_size;
>
> @@ -4252,7 +4252,7 @@ static int make_completely_empty(BlockDriverState *bs)
> offset = qcow2_alloc_clusters(bs, 3 * s->cluster_size + l1_size2);
> if (offset < 0) {
> ret = offset;
> - goto fail_broken_refcounts;
> + goto fail;
> } else if (offset > 0) {
> error_report("First cluster in emptied image is in use");
> abort();
> @@ -4274,6 +4274,9 @@ static int make_completely_empty(BlockDriverState *bs)
>
> return 0;
>
> +fail:
> + g_free(s->refcount_table);
> +
> fail_broken_refcounts:
> /* The BDS is unusable at this point. If we wanted to make it usable, we
> * would have to call qcow2_refcount_close(), qcow2_refcount_init(),
> @@ -4283,8 +4286,6 @@ fail_broken_refcounts:
> * that that sequence will fail as well. Therefore, just eject the BDS. */
> bs->drv = NULL;
>
> -fail:
> - g_free(new_reftable);
> return ret;
> }
>
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-09-03 10:24 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-31 2:04 [Qemu-devel] [PATCH] block: qcow2: free 'refcount_table' in error path Li Qiang
2019-09-03 10:22 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).