Eric Blake
2022-Aug-12 02:06 UTC
[Libguestfs] [libnbd PATCH 4/4] states: Use RESYNC to handle more structured reply server bugs
In software, it's always better to be strict in what you produce and
lenient in what you accept. Continuing on from the previous patch,
there are quite a few situations where we are expecting a particular
structured reply, but can still gracefully recover and keep the
connection alive if the server sends us something unexpected (either a
wrong reply type, or a wrong length to a correct reply type). There
are only a few situations left where we really do want a structured
reply to move to DEAD which are unrelated to read() errors on the
socket itself; those include when the server's payload is so large as
to be a denial of service, or if malloc() fails.
While touching the code, note that once we check that cmd->type is
NBD_CMD_BLOCK_STATUS, we do not also need to check whether
cmd->cb.fn.extent is non-null.
The handling for NBD_REPLY_TYPE_ERROR_* is interesting: since the
error value comes first, even if we don't get a full error packet over
the wire, we can prefer the server's errno over EPROTO. But that
means error messages should not use RESYNC except when we don't get a
full error value. This patch also changes the code to use EPROTO
instead of EINVAL if the server mistakenly sends NBD_SUCCESS as its
error code.
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:
| diff --git i/common/protocol/nbd-protocol.h w/common/protocol/nbd-protocol.h
| index e5d6404b..1e05d825 100644
| --- i/common/protocol/nbd-protocol.h
| +++ w/common/protocol/nbd-protocol.h
| @@ -242,7 +242,7 @@ struct nbd_structured_reply_error {
|
| /* Structured reply types. */
| #define NBD_REPLY_TYPE_NONE 0
| -#define NBD_REPLY_TYPE_OFFSET_DATA 1
| +#define NBD_REPLY_TYPE_OFFSET_DATA 11
| #define NBD_REPLY_TYPE_OFFSET_HOLE 2
| #define NBD_REPLY_TYPE_BLOCK_STATUS 5
| #define NBD_REPLY_TYPE_ERROR NBD_REPLY_TYPE_ERR (1)
With the following command line:
$ ./run /path/to/nbdkit -U - memory 1M --run 'nbdsh -u "$uri" -c
"\
try:
h.pread(1, 0)
except nbd.Error as ex:
print(ex.string)
h.flush()
print(h.get_size())
"'
pre-patch results show the client hanging up abruptly on the server:
nbd_pread: unknown structured reply type (11)
nbdsh: command line script failed: nbd_flush: invalid state: DEAD: the handle
must be connected with the server: Invalid argument
nbdkit: memory.1: error: read request: Connection reset by peer
while post-patch flags the server error, but allows the next command:
nbd_pread: read: command failed: Protocol error
1048576
Reading the LIBNBD_DEBUG=1 log further shows:
libnbd: debug: nbdsh: nbd_pread: unexpected reply type 11 or payload length 9
for cookie 1 and command 0, this is probably a server bug
---
generator/states-reply-structured.c | 155 +++++++++++-----------------
1 file changed, 61 insertions(+), 94 deletions(-)
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 752468c..db32873 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -94,6 +94,7 @@ STATE_MACHINE {
}
if (!h->structured_replies) {
+ resync:
h->rbuf = NULL;
h->rlen = length;
SET_NEXT_STATE (%RESYNC);
@@ -102,16 +103,8 @@ STATE_MACHINE {
switch (type) {
case NBD_REPLY_TYPE_NONE:
- if (length != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid length in NBD_REPLY_TYPE_NONE");
- break;
- }
- if (!(flags & NBD_REPLY_FLAG_DONE)) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "NBD_REPLY_FLAG_DONE must be set in
NBD_REPLY_TYPE_NONE");
- break;
- }
+ if (length != 0 || !(flags & NBD_REPLY_FLAG_DONE))
+ goto resync;
SET_NEXT_STATE (%FINISH);
break;
@@ -120,63 +113,28 @@ STATE_MACHINE {
* 0-length replies are broken. Still, it's easy enough to support
* them as an extension, so we use < instead of <=.
*/
- if (cmd->type != NBD_CMD_READ) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid command for receiving offset-data chunk,
"
- "cmd->type=%" PRIu16 ", "
- "this is likely to be a bug in the server",
- cmd->type);
- break;
- }
- 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");
- break;
- }
+ if (cmd->type != NBD_CMD_READ ||
+ length < sizeof h->sbuf.sr.payload.offset_data)
+ goto resync;
h->rbuf = &h->sbuf.sr.payload.offset_data;
h->rlen = sizeof h->sbuf.sr.payload.offset_data;
SET_NEXT_STATE (%RECV_OFFSET_DATA);
break;
case NBD_REPLY_TYPE_OFFSET_HOLE:
- if (cmd->type != NBD_CMD_READ) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid command for receiving offset-hole chunk,
"
- "cmd->type=%" PRIu16 ", "
- "this is likely to be a bug in the server",
- cmd->type);
- break;
- }
- if (length != sizeof h->sbuf.sr.payload.offset_hole) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid length in NBD_REPLY_TYPE_OFFSET_HOLE");
- break;
- }
+ if (cmd->type != NBD_CMD_READ ||
+ length != sizeof h->sbuf.sr.payload.offset_hole)
+ goto resync;
h->rbuf = &h->sbuf.sr.payload.offset_hole;
h->rlen = sizeof h->sbuf.sr.payload.offset_hole;
SET_NEXT_STATE (%RECV_OFFSET_HOLE);
break;
case NBD_REPLY_TYPE_BLOCK_STATUS:
- if (cmd->type != NBD_CMD_BLOCK_STATUS) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid command for receiving block-status chunk,
"
- "cmd->type=%" PRIu16 ", "
- "this is likely to be a bug in the server",
- cmd->type);
- break;
- }
- /* XXX We should be able to skip the bad reply in these two cases. */
- if (length < 12 || ((length-4) & 7) != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid length in NBD_REPLY_TYPE_BLOCK_STATUS");
- break;
- }
- if (CALLBACK_IS_NULL (cmd->cb.fn.extent)) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "not expecting NBD_REPLY_TYPE_BLOCK_STATUS
here");
- break;
- }
+ if (cmd->type != NBD_CMD_BLOCK_STATUS ||
+ length < 12 || ((length-4) & 7) != 0)
+ goto resync;
+ assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
/* We read the context ID followed by all the entries into a
* single array and deal with it at the end.
*/
@@ -194,25 +152,26 @@ STATE_MACHINE {
default:
if (NBD_REPLY_TYPE_IS_ERR (type)) {
- if (length < sizeof h->sbuf.sr.payload.error.error) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "too short length in structured reply error");
- break;
- }
+ /* Any payload shorter than uint32_t cannot even carry an errno
+ * value; anything longer, even if it is not long enough to be
+ * compliant, will favor the wire error over EPROTO during more
+ * length checks in RECV_ERROR_MESSAGE and RECV_ERROR_TAIL.
+ */
+ if (length < sizeof h->sbuf.sr.payload.error.error.error)
+ goto resync;
h->rbuf = &h->sbuf.sr.payload.error.error;
- h->rlen = sizeof h->sbuf.sr.payload.error.error;
+ h->rlen = MIN (length, sizeof h->sbuf.sr.payload.error.error);
SET_NEXT_STATE (%RECV_ERROR);
}
- else {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "unknown structured reply type (%" PRIu16
")", type);
- }
+ else
+ goto resync;
break;
}
return 0;
REPLY.STRUCTURED_REPLY.RECV_ERROR:
- uint32_t length, msglen;
+ struct command *cmd = h->reply_cmd;
+ uint32_t length, msglen, error;
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
@@ -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)
+ 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);
if (msglen > length - sizeof h->sbuf.sr.payload.error.error ||
- msglen > sizeof h->sbuf.sr.payload.error.msg) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "error message length too large");
- return 0;
- }
+ msglen > sizeof h->sbuf.sr.payload.error.msg)
+ goto resync;
h->rbuf = h->sbuf.sr.payload.error.msg;
h->rlen = msglen;
@@ -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)
+ 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);
@@ -300,22 +268,19 @@ STATE_MACHINE {
assert (cmd); /* guaranteed by CHECK */
/* The spec requires the server to send a non-zero error */
- if (error == NBD_SUCCESS) {
- debug (h, "server forgot to set error; using EINVAL");
- error = NBD_EINVAL;
- }
error = nbd_internal_errno_of_nbd_error (error);
+ if (error == 0) {
+ debug (h, "server forgot to set error; using EPROTO");
+ error = EPROTO;
+ }
/* Sanity check that any error offset is in range, then invoke
- * user callback if present.
+ * user callback if present. Ignore the offset if it was bogus.
*/
- if (type == NBD_REPLY_TYPE_ERROR_OFFSET) {
+ if (type == NBD_REPLY_TYPE_ERROR_OFFSET && h->rbuf) {
offset = be64toh (h->sbuf.sr.payload.error.offset);
- if (! structured_reply_in_bounds (offset, 0, cmd)) {
- SET_NEXT_STATE (%.DEAD);
- return 0;
- }
- if (cmd->type == NBD_CMD_READ &&
+ if (structured_reply_in_bounds (offset, 0, cmd) &&
+ cmd->type == NBD_CMD_READ &&
CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
int scratch = error;
@@ -330,6 +295,8 @@ STATE_MACHINE {
if (cmd->error == 0)
cmd->error = scratch;
}
+ else
+ debug (h, "no use for error offset %" PRIu64, offset);
}
/* Preserve first error encountered */
--
2.37.1
Richard W.M. Jones
2022-Aug-12 08:03 UTC
[Libguestfs] [libnbd PATCH 4/4] states: Use RESYNC to handle more structured reply server bugs
The series looks sensible, I didn't review every line but I read all the changes and it seems OK. I think what I will do when the heatwave breaks next week is to fire up AFL++ and get it to check that there are no obvious issues caused after this series is upstream. I will first need to check that our test cases use both simple & structured replies, or if not generate the missing test case. Series: Acked-by: Richard W.M. Jones <rjones at redhat.com> Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Eric Blake
2022-Aug-12 13:07 UTC
[Libguestfs] [libnbd PATCH 4/4] states: Use RESYNC to handle more structured reply server bugs
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