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

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(a)redhat.com&gt; > --- > 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.