Re: [Qemu-stable] [Qemu-block] [PATCH v5 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Stefan Hajnoczi]

On Tue, May 05, 2015 at 10:51:14AM +0800, Fam Zheng wrote:

This function is complex.  I had to draw a diagram to remember the
relationships between the variables.  It would be nice to split it if
that can be done in a way that makes the code nicer.

> @@ -1236,13 +1238,39 @@ static int coroutine_fn 
> bdrv_co_do_pwritev(BlockDriverState *bs,
>          }
>          BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>  
> -        qemu_iovec_init(&local_qiov, qiov->niov + 2);
> -        qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
> -        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> -        use_local_qiov = true;
> +        if (qiov) {
> +            qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 1);

Should be:
qemu_iovec_init(&local_qiov, qiov->niov + 2);

since we know qiov != NULL here.

> @@ -1270,21 +1298,39 @@ static int coroutine_fn 
> bdrv_co_do_pwritev(BlockDriverState *bs,
>          }
>          BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>  
> -        if (!use_local_qiov) {
> -            qemu_iovec_init(&local_qiov, qiov->niov + 1);
> -            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> -            use_local_qiov = true;
> +        if (qiov) {
> +            if (!use_local_qiov) {
> +                qemu_iovec_init(&local_qiov, qiov->niov + 1);
> +                qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> +                use_local_qiov = true;
> +            }
> +
> +            tail_bytes = (offset + bytes) & (align - 1);
> +            qemu_iovec_add(&local_qiov, tail_buf + tail_bytes,
> +                           align - tail_bytes);
> +
> +            bytes = ROUND_UP(bytes, align);
> +        } else {
> +            assert((offset & (align - 1)) == 0);
> +            assert(bytes < align);
> +
> +            memset(tail_buf, 0, bytes & (align - 1));

Since assert(bytes < align) is above, the following is clearer:

memset(tail_buf, 0, bytes)

pgpAiay8FNnqz.pgp
Description: PGP signature