Libguestfs - Jan 2022 - [v2v PATCH 1/2] lib/utils: get_disk_allocated: assert that NBD block length is positive

It makes no sense for the NBD server to return a zero-length block, plus
it used to be a bug in the libnbd OCaml bindings to wrap 32-bit block
lengths with the MSB set around to negative (signed) 32-bit integers
(which would then be widened to negative (signed) 64-bit integers).

Any non-positive "len" value breaks the progression of
"fetch_offset",
potentially leading to an infinite loop.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/utils.ml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/utils.ml b/lib/utils.ml
index 4c43a4b5161d..f599b0e32450 100644
--- a/lib/utils.ml
+++ b/lib/utils.ml
@@ -197,6 +197,7 @@ let get_disk_allocated ~dir ~disknr                    for i
= 0 to Array.length entries / 2 - 1 do
                     let len = Int64.of_int32 entries.(i * 2)
                     and typ = entries.(i * 2 + 1) in
+                    assert (len > 0_L);
                     if Int32.logand typ 1_l = 0_l then
                       allocated := !allocated +^ len;
                     fetch_offset := !fetch_offset +^ len
-- 
2.19.1.3.g30247aa5d201

On Fri, Jan 14, 2022 at 02:50:47PM +0100, Laszlo Ersek
wrote:
> It makes no sense for the NBD server to return a zero-length block, plus
> it used to be a bug in the libnbd OCaml bindings to wrap 32-bit block
> lengths with the MSB set around to negative (signed) 32-bit integers
> (which would then be widened to negative (signed) 64-bit integers).
> 
> Any non-positive "len" value breaks the progression of
"fetch_offset",
> potentially leading to an infinite loop.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  lib/utils.ml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/utils.ml b/lib/utils.ml
> index 4c43a4b5161d..f599b0e32450 100644
> --- a/lib/utils.ml
> +++ b/lib/utils.ml
> @@ -197,6 +197,7 @@ let get_disk_allocated ~dir ~disknr >               
for i = 0 to Array.length entries / 2 - 1 do
>                      let len = Int64.of_int32 entries.(i * 2)
>                      and typ = entries.(i * 2 + 1) in
> +                    assert (len > 0_L);
>                      if Int32.logand typ 1_l = 0_l then
>                        allocated := !allocated +^ len;
>                      fetch_offset := !fetch_offset +^ len
> -- 
> 2.19.1.3.g30247aa5d201
I'm inclined to ACK, but it does leave us open to the possibility that
virt-v2v will crash if an NBD server returned a zero length block.
That would be sort of wrong, but could still happen in a
semi-well-behaved custom nbdkit plugin which is returning other blocks
and so still making process.  Eric: any thoughts on this?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v