src may be NULL if BDRV_REQ_ZERO_WRITE flag is set, in this case only
check dst and dst->bs. This bug was introduced when moving in the
request tracking code from bdrv_co_copy_range, in 37aec7d75eb.

This especially fixes the possible segfault when initializing src_bs
with a NULL src.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7035b78a20..b8845708d7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2897,18 +2897,11 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
                                                     bool recurse_src)
 {
     BdrvTrackedRequest src_req, dst_req;
-    BlockDriverState *src_bs = src->bs;
-    BlockDriverState *dst_bs = dst->bs;
     int ret;
 
-    if (!src || !dst || !src->bs || !dst->bs) {
+    if (!dst || !dst->bs) {
         return -ENOMEDIUM;
     }
-    ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
-    if (ret) {
-        return ret;
-    }
-
     ret = bdrv_check_byte_request(dst->bs, dst_offset, bytes);
     if (ret) {
         return ret;
@@ -2917,16 +2910,24 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
         return bdrv_co_pwrite_zeroes(dst, dst_offset, bytes, flags);
     }
 
+    if (!src || !src->bs) {
+        return -ENOMEDIUM;
+    }
+    ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
+    if (ret) {
+        return ret;
+    }
+
     if (!src->bs->drv->bdrv_co_copy_range_from
         || !dst->bs->drv->bdrv_co_copy_range_to
         || src->bs->encrypted || dst->bs->encrypted) {
         return -ENOTSUP;
     }
-    bdrv_inc_in_flight(src_bs);
-    bdrv_inc_in_flight(dst_bs);
-    tracked_request_begin(&src_req, src_bs, src_offset,
+    bdrv_inc_in_flight(src->bs);
+    bdrv_inc_in_flight(dst->bs);
+    tracked_request_begin(&src_req, src->bs, src_offset,
                           bytes, BDRV_TRACKED_READ);
-    tracked_request_begin(&dst_req, dst_bs, dst_offset,
+    tracked_request_begin(&dst_req, dst->bs, dst_offset,
                           bytes, BDRV_TRACKED_WRITE);
 
     wait_serialising_requests(&src_req);
@@ -2944,8 +2945,8 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
     }
     tracked_request_end(&src_req);
     tracked_request_end(&dst_req);
-    bdrv_dec_in_flight(src_bs);
-    bdrv_dec_in_flight(dst_bs);
+    bdrv_dec_in_flight(src->bs);
+    bdrv_dec_in_flight(dst->bs);
     return ret;
 }
 
-- 
2.17.1

On Tue, Jul 03, 2018 at 10:37:56AM +0800, Fam Zheng wrote:
> src may be NULL if BDRV_REQ_ZERO_WRITE flag is set, in this case only
> check dst and dst->bs. This bug was introduced when moving in the
> request tracking code from bdrv_co_copy_range, in 37aec7d75eb.
> 
> This especially fixes the possible segfault when initializing src_bs
> with a NULL src.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/io.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 7035b78a20..b8845708d7 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2897,18 +2897,11 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
>                                                      bool recurse_src)
>  {
>      BdrvTrackedRequest src_req, dst_req;
> -    BlockDriverState *src_bs = src->bs;
> -    BlockDriverState *dst_bs = dst->bs;
>      int ret;
>  
> -    if (!src || !dst || !src->bs || !dst->bs) {
> +    if (!dst || !dst->bs) {
>          return -ENOMEDIUM;
>      }
> -    ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
> -    if (ret) {
> -        return ret;
> -    }
> -
>      ret = bdrv_check_byte_request(dst->bs, dst_offset, bytes);
>      if (ret) {
>          return ret;
> @@ -2917,16 +2910,24 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
>          return bdrv_co_pwrite_zeroes(dst, dst_offset, bytes, flags);
>      }
>  
> +    if (!src || !src->bs) {
> +        return -ENOMEDIUM;
> +    }
> +    ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
> +    if (ret) {
> +        return ret;
> +    }
> +
>      if (!src->bs->drv->bdrv_co_copy_range_from
>          || !dst->bs->drv->bdrv_co_copy_range_to
>          || src->bs->encrypted || dst->bs->encrypted) {
>          return -ENOTSUP;
>      }
> -    bdrv_inc_in_flight(src_bs);
> -    bdrv_inc_in_flight(dst_bs);
> -    tracked_request_begin(&src_req, src_bs, src_offset,
> +    bdrv_inc_in_flight(src->bs);
> +    bdrv_inc_in_flight(dst->bs);
> +    tracked_request_begin(&src_req, src->bs, src_offset,
>                            bytes, BDRV_TRACKED_READ);
> -    tracked_request_begin(&dst_req, dst_bs, dst_offset,
> +    tracked_request_begin(&dst_req, dst->bs, dst_offset,
>                            bytes, BDRV_TRACKED_WRITE);
>  
>      wait_serialising_requests(&src_req);
> @@ -2944,8 +2945,8 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
>      }
>      tracked_request_end(&src_req);
>      tracked_request_end(&dst_req);
> -    bdrv_dec_in_flight(src_bs);
> -    bdrv_dec_in_flight(dst_bs);
> +    bdrv_dec_in_flight(src->bs);
> +    bdrv_dec_in_flight(dst->bs);
>      return ret;
>  }
>  
> -- 
> 2.17.1
>