Libguestfs - Jul 2023 - [libnbd PATCH v4 6/6] states: Break deadlock if server goofs on extended replies

One of the benefits of extended replies is that we can do a
fixed-length read for the entire header of every server reply, which
is fewer syscalls than the split-read approach required by structured
replies.  But one of the drawbacks of doing a large read is that if
the server is non-compliant (not a problem for normal servers, but
something I hit rather more than I'd like to admit while developing
extended header support in servers), nbd_pwrite() and friends will
deadlock if the server replies with the wrong header.  Add in some
code to catch that failure mode and move the state machine to DEAD
sooner, to make it easier to diagnose the fault in the server.

Unlike in the case of an unexpected simple reply from a structured
server (where, since b31e7bac, we can merely fail the command with
EPROTO and successfully move on to the next server reply, because we
didn't read too many bytes yet), in this case we really do have to
move to DEAD: we cannot assume our short read was just the 16 (simple)
or 20 (structured) bytes that the server sent for this command,
because it is also possible that we can see a short read even while
reading two back-to-back replies; if we have already read the initial
bytes of the server's next reply, we have no way to push those extra
bytes back onto our read stream for parsing on our next pass through
the state machine.

Signed-off-by: Eric Blake <eblake at redhat.com>
---

v4: rebase to master, improve comment accuracy [Laszlo], drop bogus #if 0
---
 generator/states-reply.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/generator/states-reply.c b/generator/states-reply.c
index 05301ae8..9f0918e2 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -126,7 +126,25 @@  REPLY.START:
  REPLY.RECV_REPLY:
   switch (recv_into_rbuf (h)) {
   case -1: SET_NEXT_STATE (%.DEAD); return 0;
-  case 1: SET_NEXT_STATE (%.READY); return 0;
+  case 1: SET_NEXT_STATE (%.READY);
+    /* Special case: if we have a short read, but got at least far
+     * enough to decode the magic number, we can check if the server
+     * is matching our expectations. This lets us avoid deadlocking if
+     * we are blocked waiting for a 32-byte extended reply, while a
+     * buggy server only sent a shorter simple or structured reply.
+     * Magic number checks here must be repeated in CHECK_REPLY_MAGIC,
+     * since we do not always encounter a short read.
+     */
+    if (h->extended_headers &&
+        (char *)h->rbuf >+        (char *)&h->sbuf.reply.hdr +
sizeof h->sbuf.reply.hdr.magic) {
+      uint32_t magic = be32toh (h->sbuf.reply.hdr.magic);
+      if (magic != NBD_EXTENDED_REPLY_MAGIC) {
+        SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */
+        set_error (0, "invalid or unexpected reply magic 0x%" PRIx32,
magic);
+      }
+    }
+    return 0;
   case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC);
   }
   return 0;
-- 
2.41.0

On 7/21/23 18:08, Eric Blake wrote:
> One of the benefits of extended replies is that we can do a
> fixed-length read for the entire header of every server reply, which
> is fewer syscalls than the split-read approach required by structured
> replies.  But one of the drawbacks of doing a large read is that if
> the server is non-compliant (not a problem for normal servers, but
> something I hit rather more than I'd like to admit while developing
> extended header support in servers), nbd_pwrite() and friends will
> deadlock if the server replies with the wrong header.  Add in some
> code to catch that failure mode and move the state machine to DEAD
> sooner, to make it easier to diagnose the fault in the server.
> 
> Unlike in the case of an unexpected simple reply from a structured
> server (where, since b31e7bac, we can merely fail the command with
> EPROTO and successfully move on to the next server reply, because we
> didn't read too many bytes yet), in this case we really do have to
> move to DEAD: we cannot assume our short read was just the 16 (simple)
> or 20 (structured) bytes that the server sent for this command,
> because it is also possible that we can see a short read even while
> reading two back-to-back replies; if we have already read the initial
> bytes of the server's next reply, we have no way to push those extra
> bytes back onto our read stream for parsing on our next pass through
> the state machine.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> v4: rebase to master, improve comment accuracy [Laszlo], drop bogus #if 0
> ---
>  generator/states-reply.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 05301ae8..9f0918e2 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -126,7 +126,25 @@  REPLY.START:
>   REPLY.RECV_REPLY:
>    switch (recv_into_rbuf (h)) {
>    case -1: SET_NEXT_STATE (%.DEAD); return 0;
> -  case 1: SET_NEXT_STATE (%.READY); return 0;
> +  case 1: SET_NEXT_STATE (%.READY);
> +    /* Special case: if we have a short read, but got at least far
> +     * enough to decode the magic number, we can check if the server
> +     * is matching our expectations. This lets us avoid deadlocking if
> +     * we are blocked waiting for a 32-byte extended reply, while a
> +     * buggy server only sent a shorter simple or structured reply.
> +     * Magic number checks here must be repeated in CHECK_REPLY_MAGIC,
> +     * since we do not always encounter a short read.
> +     */
> +    if (h->extended_headers &&
> +        (char *)h->rbuf >> +        (char
*)&h->sbuf.reply.hdr + sizeof h->sbuf.reply.hdr.magic) {
> +      uint32_t magic = be32toh (h->sbuf.reply.hdr.magic);
> +      if (magic != NBD_EXTENDED_REPLY_MAGIC) {
> +        SET_NEXT_STATE (%.DEAD); /* We've probably lost
synchronization. */
> +        set_error (0, "invalid or unexpected reply magic 0x%"
PRIx32, magic);
> +      }
> +    }
> +    return 0;
>    case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC);
>    }
>    return 0;
Acked-by: Laszlo Ersek <lersek at redhat.com>