Re: [Qemu-devel] [PATCH 3/4] qed: Make qiov match request size until backing file EOF

[Kevin Wolf]

Am 05.07.2014 um 22:06 hat Max Reitz geschrieben:
> On 04.07.2014 17:55, Kevin Wolf wrote:
> >If a QED image has a shorter backing file and a read request to
> >unallocated clusters goes across EOF of the backing file, the backing
> >file sees a shortened request and the rest is filled with zeros.
> >However, the original too long qiov was used with the shortened request.
> >
> >This patch makes the qiov size match the request size, avoiding a
> >potential buffer overflow in raw-posix.
> >
> >Signed-off-by: Kevin Wolf <address@hidden>
> >---
> >  block/qed.c | 26 +++++++++++++++++++++++---
> >  block/qed.h |  1 +
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/qed.c b/block/qed.c
> >index b69374b..1f63b8f 100644
> >--- a/block/qed.c
> >+++ b/block/qed.c
> >@@ -772,6 +772,7 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
> >   */
> >  static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
> >                                    QEMUIOVector *qiov,
> >+                                  QEMUIOVector **backing_qiov,
> 
> This could be documented in the comment above the function header.
> 
> >                                    BlockDriverCompletionFunc *cb, void 
> > *opaque)
> >  {
> >      uint64_t backing_length = 0;
> >@@ -804,15 +805,20 @@ static void qed_read_backing_file(BDRVQEDState *s, 
> >uint64_t pos,
> >      /* If the read straddles the end of the backing file, shorten it */
> >      size = MIN((uint64_t)backing_length - pos, qiov->size);
> >+    *backing_qiov = g_new(QEMUIOVector, 1);
> 
> I guess at least because of the qemu_iovec_destroy() block in
> qed_aio_next_io() *backing_qiov always has to be NULL before this
> point. I guess I'd like an assert(!*backing_qiov) here (or
> assert(!acb->backing_qiov) before the call to
> qed_read_backing_file() in qed_aio_read_data()) anyway to express
> clearly that there can be no leaks.

Okay, I'll add the suggested comment and assertion.

> Speaking of leaks: Shouldn't the backing_qiov be freed in
> qed_aio_complete()?

I can't see a code path where cb (i.e. qed_aio_next_io or
qed_copy_from_backing_file_write) wouldn't be called before
reaching qed_aio_complete(). Both of them free backing_qiov.

Kevin