[Libguestfs] [libnbd PATCH 3/8] pread: Reject server SR read response with no data chunks

[Eric Blake]

On 6/20/19 12:49 AM, Richard W.M. Jones wrote:
> On Mon, Jun 17, 2019 at 07:07:53PM -0500, Eric Blake wrote:
>> The NBD spec requires that a server doing structured reads must not
>> succeed unless it covers the entire buffer with reply chunks. In the
>> general case, this requires a lot of bookkeeping to check whether
>> offsets were non-overlapping and sufficient, and we'd rather defer
>> such checking to an optional callback function. But a callback
>> function will only be reached per chunk, while we still want to fail
>> the overall read if the callback function was never called because the
>> server erroneously replied with NBD_REPLY_TYPE_NONE with no other
>> chunks instead of an expected NBD_REPLY_TYPE_ERROR*. For this specific
>> error case, the bookkeeping is much simpler - we merely track if we've
>> seen at least one data chunk.
> 
> I guess the implicit assumption here is that count > 0, which IIRC the
> NBD protocol demands.  It seems like we don't actually check this in
> nbd_internal_command_common so would it be worth adding a check there?

The protocol says a 0-length read request is unspecified behavior. I
don't know if nbd_internal_command_common should reject it - a
particular server may have reliable behavior, and thus libnbd should not
prevent a client from doing it. On the other hand, the spec says a
0-length structured read chunk is broken. In qemu-nbd, a 0-length read
is not forbidden, but it results in a mere NBD_REPLY_TYPE_DONE rather
than an NBD_REPLY_TYPE_OFFSET_DATA.  Which means this patch should
probably special case a 0 length request to NOT be an error even when
data_seen is false.

We may someday want to add a strict mode (where we do as much as we can
to prevent clients from sending 0-length or out-of-bounds requests, and
do additional sanity checking that server replies are compliant), vs. a
loose mode (where clients can send malformed requests, in order to test
server responses). This is yet one more place to consider gating under
such an option setting.

> 
> In any case this patch on its own looks good, so ACK.

Here's the tweak I'm adding before pushing:

diff --git i/generator/states-reply-structured.c
w/generator/states-reply-structured.c
index 6740400..9bde46f 100644
--- i/generator/states-reply-structured.c
+++ w/generator/states-reply-structured.c
@@ -95,6 +95,10 @@
     return 0;
   }
   else if (type == NBD_REPLY_TYPE_OFFSET_DATA) {
+    /* The spec states that 0-length requests are unspecified, but
+     * 0-length replies are broken. Still, it's easy enough to support
+     * them as an extension, so we use < instead of <=.
+     */
     if (length < sizeof h->sbuf.sr.payload.offset_data) {
       SET_NEXT_STATE (%.DEAD);
       set_error (0, "too short length in NBD_REPLY_TYPE_OFFSET_DATA");
@@ -110,9 +110,9 @@
     return 0;
   }
   else if (type == NBD_REPLY_TYPE_OFFSET_HOLE) {
-    if (length != 12) {
+    if (length != sizeof h->sbuf.sr.payload.offset_hole) {
       SET_NEXT_STATE (%.DEAD);
-      set_error (0, "invalid length in NBD_REPLY_TYPE_NONE");
+      set_error (0, "invalid length in NBD_REPLY_TYPE_OFFSET_HOLE");
       return -1;
     }
     h->rbuf = &h->sbuf.sr.payload.offset_hole;
@@ -356,6 +360,10 @@
       return -1;
     }

+    /* The spec states that 0-length requests are unspecified, but
+     * 0-length replies are broken. Still, it's easy enough to support
+     * them as an extension, and this works even when length == 0.
+     */
     memset (cmd->data + offset, 0, length);

     SET_NEXT_STATE(%FINISH);
diff --git i/lib/aio.c w/lib/aio.c
index 7fb0fdf..2dcce68 100644
--- i/lib/aio.c
+++ w/lib/aio.c
@@ -73,7 +73,10 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,

   type = cmd->type;
   error = cmd->error;
-  if (type == NBD_CMD_READ && !cmd->data_seen && !error)
+  /* The spec states that a 0-length read request is unspecified; but
+   * it is easy enough to treat it as successful as an extension.
+   */
+  if (type == NBD_CMD_READ && !cmd->data_seen && cmd->count && !error)
     error = EIO;

   /* Retire it from the list and free it. */

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190620/7e411854/attachment.sig>