[Qemu-devel] [PATCH] avoid core reading with bdrv_read (qemu-io)

[Qemu-devel] [PATCH] avoid core reading with bdrv_read (qemu-io)

[Frediano Ziglio]

This patch apply to kevin coroutine-block branch and avoid code. It
fix "qcow: Use coroutines" patch. Test case:

$ ./qemu-img create -f qcow aaa.img 1G
Formatting 'aaa.img', fmt=qcow size=1073741824 encryption=off
$ ./qemu-io aaa.img
qemu-io> read 1024 1024
Segmentation fault

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
 block/qcow.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 6f7973c..1386e92 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -573,7 +573,8 @@ static int qcow_aio_read_cb(void *opaque)

     if (acb->nb_sectors == 0) {
         /* request completed */
-        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
+        if (acb->orig_buf)
+            qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
         return 0;
     }

@@ -648,6 +649,7 @@ static int qcow_co_readv(BlockDriverState *bs,
int64_t sector_num,

     if (acb->qiov->niov > 1) {
         qemu_vfree(acb->orig_buf);
+        acb->orig_buf = NULL;
     }
     qemu_aio_release(acb);

@@ -729,6 +731,7 @@ static int qcow_co_writev(BlockDriverState *bs,
int64_t sector_num,

     if (acb->qiov->niov > 1) {
         qemu_vfree(acb->orig_buf);
+        acb->orig_buf = NULL;
     }
     qemu_aio_release(acb);

-- 
1.7.1

Re: [Qemu-devel] [PATCH] avoid core reading with bdrv_read (qemu-io)

[Kevin Wolf]

Am 19.07.2011 09:33, schrieb Frediano Ziglio:
> This patch apply to kevin coroutine-block branch and avoid code. It
> fix "qcow: Use coroutines" patch. Test case:
> 
> $ ./qemu-img create -f qcow aaa.img 1G
> Formatting 'aaa.img', fmt=qcow size=1073741824 encryption=off
> $ ./qemu-io aaa.img
> qemu-io> read 1024 1024
> Segmentation fault
> 
> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>

Thanks for the report. I'll update the patch, but in a slightly
different way that matches the old code better:

diff --git a/block/qcow.c b/block/qcow.c
index 6f7973c..6447c2a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -573,7 +573,6 @@ static int qcow_aio_read_cb(void *opaque)

     if (acb->nb_sectors == 0) {
         /* request completed */
-        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
         return 0;
     }

@@ -647,6 +646,7 @@ static int qcow_co_readv(BlockDriverState *bs,
int64_t sector_num,
     qemu_co_mutex_unlock(&s->lock);

     if (acb->qiov->niov > 1) {
+        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
         qemu_vfree(acb->orig_buf);
     }
     qemu_aio_release(acb);

Re: [Qemu-devel] [PATCH] avoid core reading with bdrv_read (qemu-io)

[Frediano Ziglio]

2011/7/19 Kevin Wolf <kwolf@redhat.com>:
> Am 19.07.2011 09:33, schrieb Frediano Ziglio:
>> This patch apply to kevin coroutine-block branch and avoid code. It
>> fix "qcow: Use coroutines" patch. Test case:
>>
>> $ ./qemu-img create -f qcow aaa.img 1G
>> Formatting 'aaa.img', fmt=qcow size=1073741824 encryption=off
>> $ ./qemu-io aaa.img
>> qemu-io> read 1024 1024
>> Segmentation fault
>>
>> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
>
> Thanks for the report. I'll update the patch, but in a slightly
> different way that matches the old code better:
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 6f7973c..6447c2a 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -573,7 +573,6 @@ static int qcow_aio_read_cb(void *opaque)
>
>     if (acb->nb_sectors == 0) {
>         /* request completed */
> -        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
>         return 0;
>     }
>
> @@ -647,6 +646,7 @@ static int qcow_co_readv(BlockDriverState *bs,
> int64_t sector_num,
>     qemu_co_mutex_unlock(&s->lock);
>
>     if (acb->qiov->niov > 1) {
> +        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
>         qemu_vfree(acb->orig_buf);
>     }
>     qemu_aio_release(acb);
>

Yes, my patch also removed some dandling pointer which I don't like
but are not a problem with current code.
In case of ret < 0 (error) your code could copy data that probably are
not initialized. I don't know if data is used in case of failure but
in case memory is shared with guest (I don't know, perhaps using
virtio) this lead to security issues. Also some memory debugger like
valgrind could not like that copy.

Frediano