9:06 p.m.
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