On Thu, Aug 11, 2022 at 09:06:38PM -0500, Eric Blake wrote:
Again, compliant servers will never trip over to the new state; but an easy way to demonstrate this in action is with a temporary patch to nbdkit:
That tested one of the error paths; I found a few others where the patch needed tweaks:
@@ -222,13 +181,25 @@ STATE_MACHINE { return 0; case 0: length = be32toh (h->sbuf.sr.structured_reply.length); + assert (length >= sizeof h->sbuf.sr.payload.error.error.error); + assert (cmd); + + if (length < sizeof h->sbuf.sr.payload.error.error) { + resync: + /* Favor the error packet's errno over RESYNC's EPROTO. */ + error = be32toh (h->sbuf.sr.payload.error.error.error); + if (cmd->error = 0)
Typo: that should be == rather than =.
+ cmd->error = nbd_internal_errno_of_nbd_error (error); + h->rbuf = NULL; + h->rlen = length - MIN (length, sizeof h->sbuf.sr.payload.error.error); + SET_NEXT_STATE (%RESYNC); + return 0; + } + msglen = be16toh (h->sbuf.sr.payload.error.error.len); ...
@@ -257,24 +228,21 @@ STATE_MACHINE { debug (h, "structured error server message: %.*s", (int) msglen, h->sbuf.sr.payload.error.msg);
- /* Special case two specific errors; ignore the tail for all others */ + /* Special case two specific errors; silently ignore tail for all others */ h->rbuf = NULL; h->rlen = length; switch (type) { case NBD_REPLY_TYPE_ERROR: - if (length != 0) { - SET_NEXT_STATE (%.DEAD); - set_error (0, "error payload length too large"); - return 0; - } + if (length != 0) + debug (h, "ignoring unexpected slop after error message, " + "the server may have a bug"); break; case NBD_REPLY_TYPE_ERROR_OFFSET: - if (length != sizeof h->sbuf.sr.payload.error.offset) { - SET_NEXT_STATE (%.DEAD); - set_error (0, "invalid error payload length"); - return 0; - } - h->rbuf = &h->sbuf.sr.payload.error.offset; + if (length == sizeof h->sbuf.sr.payload.error.offset)
and that should be !=, not ==.
+ debug (h, "unable to safely extract error offset, " + "the server may have a bug"); + else + h->rbuf = &h->sbuf.sr.payload.error.offset; break; } SET_NEXT_STATE (%RECV_ERROR_TAIL);
Plus a typo in the subject for 1/4. With everything fixed, the series is now in as 185195d..0883029. Another leniency issue I am exploring is whether structured reply mode can cope with a server that mistakenly sends a simply reply to NBD_CMD_READ, and whether the client can ignore replies for an unknown cookie. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org