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 <ebl...@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 <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
