4:53 p.m.
I'm still playing with ideas on how to split rstate from wstate (so
that we can send a request without waiting for POLLIN to complete a
pending reply), but this is some preliminary refactoring I found
useful. I also fixed a couple of bugs while in the area (already
pushed).
There's a question of whether we want nbd_handle to be nearly 5k, or
if we should instead keep it small and add one more indirection where
sbuf is malloc'd; I'm not sure that it matters much (outside of
libnbd, nbd_handle is an opaque type always referenced as a pointer).
Eric Blake (7):
states: Log structured error messages, if any
states: Refactor SET_META_CONTEXT reply parsing
states: Allow large SET_CONTEXT_NAME replies
states: Rewrite NBD_REP_INFO parsing
states: Factor out NBD_REP payload prep
states: Give up on oversized reply length
states: Capture NBD_REP_ERR message
generator/generator | 19 ++--
generator/states-newstyle-opt-go.c | 71 ++++++-------
.../states-newstyle-opt-set-meta-context.c | 99 +++++++------------
generator/states-newstyle-opt-starttls.c | 39 ++------
.../states-newstyle-opt-structured-reply.c | 34 +++----
generator/states-newstyle.c | 90 +++++++++++++++++
generator/states-reply-structured.c | 23 ++++-
lib/internal.h | 4 +-
8 files changed, 207 insertions(+), 172 deletions(-)
--
2.20.1
4:53 p.m.
New subject: [libnbd PATCH 1/7] states: Log structured error messages, if any
This increases the size of struct nbd_handle from about 700 bytes to
instead being just under 5k; that's not too bad of a cost, and future
patches will reuse same storage for logging any error messages during
handshake.
---
generator/states-reply-structured.c | 10 +++++++---
lib/internal.h | 1 +
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 399f5e2..694c282 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -156,14 +156,14 @@
case 0:
length = be32toh (h->sbuf.sr.structured_reply.length);
msglen = be16toh (h->sbuf.sr.payload.error.len);
- if (msglen > length - sizeof h->sbuf.sr.payload.error) {
+ if (msglen > length - sizeof h->sbuf.sr.payload.error ||
+ msglen > sizeof h->sbuf.sr.err_msg) {
SET_NEXT_STATE (%.DEAD);
set_error (0, "error message length too large");
return -1;
}
- /* We skip the human readable error for now. XXX */
- h->rbuf = NULL;
+ h->rbuf = h->sbuf.sr.err_msg;
h->rlen = msglen;
SET_NEXT_STATE (%RECV_ERROR_MESSAGE);
}
@@ -182,6 +182,10 @@
length -= sizeof h->sbuf.sr.payload.error - msglen;
+ if (msglen)
+ debug (h, "structured error server message: %.*s", (int) msglen,
+ h->sbuf.sr.err_msg);
+
/* Special case two specific errors; ignore the tail for all others */
h->rbuf = NULL;
h->rlen = length;
diff --git a/lib/internal.h b/lib/internal.h
index f5b1f95..6750938 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -138,6 +138,7 @@ struct nbd_handle {
struct nbd_structured_reply_offset_hole offset_hole;
struct nbd_structured_reply_error error;
} payload;
+ char err_msg[NBD_MAX_STRING];
} __attribute__((packed)) sr;
uint32_t cflags;
uint32_t len;
--
2.20.1
4:53 p.m.
New subject: [libnbd PATCH 2/7] states: Refactor SET_META_CONTEXT reply parsing
We had two different flows for a loop ended by error/NBD_REP_ACK:
OPT_GO OPT_SET_META_CONTEXT
- RECV_REPLY - RECV_REPLY
finish read() of reply header finish read() of reply header
set up to read/skip replylen bytes sanity check header
- RECV_REPLY_PAYLOAD switch based on reply type
finish read()/skip of reply payload set up read/skip replylen bytes
- CHECK_REPLY - RECV_REPLY_PAYLOAD
sanity check header finish read() of reply payload
switch based on reply type utilize payload
utilize pre-parsed payload choose next state
choose next state - SKIP_PAYLOAD
finish skip of reply payload
- FINISH
go to next state
I'm finding the OPT_GO flow easier to understand - it has fewer
states, and separates collecting the data from parsing it rather than
splitting the parse across two states. It is also a better idea to
collect reply payload for all reply types, not just our expected one,
if we are to change things to start reporting any server's error
messages appended to NBD_REP_ERR.
The OPT_STARTTLS and OPT_STRUCTURED_REPLY sub-states are also similar
to OPT_GO, except that they currently always skip rather than read()
the payload.
The patch feels rather large; view with 'git diff -b' for a slightly
easier read. But there should be no change in behavior other than a
new debug message when we skip a replied context for being too large.
---
generator/generator | 11 +-
.../states-newstyle-opt-set-meta-context.c | 120 +++++++++---------
2 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/generator/generator b/generator/generator
index deb77f0..e3dd10f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -519,15 +519,8 @@ and newstyle_opt_set_meta_context_state_machine = [
State {
default_state with
- name = "RECV_SKIP_PAYLOAD";
- comment = "Ignore newstyle NBD_OPT_SET_META_CONTEXT option reply payload";
- external_events = [ NotifyRead, "" ];
- };
-
- State {
- default_state with
- name = "FINISH";
- comment = "Finish newstyle NBD_OPT_SET_META_CONTEXT option parsing";
+ name = "CHECK_REPLY";
+ comment = "Check newstyle NBD_OPT_SET_META_CONTEXT option reply";
external_events = [];
};
]
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index 6b5a6d6..feaa295 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -30,7 +30,7 @@
if (!h->structured_replies ||
h->request_meta_contexts == NULL ||
nbd_internal_string_list_length (h->request_meta_contexts) == 0) {
- SET_NEXT_STATE (%FINISH);
+ SET_NEXT_STATE (%^OPT_GO.START);
return 0;
}
@@ -139,67 +139,68 @@
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY:
- uint64_t magic;
- uint32_t option;
- uint32_t reply;
uint32_t len;
- const uint32_t maxlen = sizeof h->sbuf.or.payload.context;
+ const uint32_t maxpayload = sizeof h->sbuf.or.payload.context;
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
- reply = be32toh (h->sbuf.or.option_reply.reply);
+ /* Read the following payload if it is short enough to fit in the
+ * static buffer. If it's too long, skip it.
+ */
len = be32toh (h->sbuf.or.option_reply.replylen);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_SET_META_CONTEXT) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
- switch (reply) {
- case NBD_REP_ACK: /* End of list of replies. */
- if (len != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
- SET_NEXT_STATE (%FINISH);
- break;
- case NBD_REP_META_CONTEXT: /* A context. */
- /* If it's too long, skip over it. */
- if (len > maxlen)
- h->rbuf = NULL;
- else if (len <= sizeof h->sbuf.or.payload.context.context) {
- /* A valid reply has at least one byte in payload.context.str */
- set_error (0, "NBD_REP_META_CONTEXT reply length too small");
- SET_NEXT_STATE (%.DEAD);
- return -1;
- }
- else
- h->rbuf = &h->sbuf.or.payload.context;
- h->rlen = len;
- SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
- break;
- default:
- /* Anything else is an error, ignore it */
- debug (h, "handshake: unexpected error from "
- "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
+ if (len <= maxpayload)
+ h->rbuf = &h->sbuf.or.payload;
+ else
h->rbuf = NULL;
- h->rlen = len;
- SET_NEXT_STATE (%RECV_SKIP_PAYLOAD);
- }
+ h->rlen = len;
+ SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY_PAYLOAD:
+ switch (recv_into_rbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return -1;
+ case 0: SET_NEXT_STATE (%CHECK_REPLY);
+ }
+ return 0;
+
+ NEWSTYLE.OPT_SET_META_CONTEXT.CHECK_REPLY:
+ uint64_t magic;
+ uint32_t option;
+ uint32_t reply;
+ uint32_t len;
+ const size_t maxpayload = sizeof h->sbuf.or.payload.context;
struct meta_context *meta_context;
- uint32_t len;
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return -1;
- case 0:
- if (h->rbuf != NULL) {
+ magic = be64toh (h->sbuf.or.option_reply.magic);
+ option = be32toh (h->sbuf.or.option_reply.option);
+ reply = be32toh (h->sbuf.or.option_reply.reply);
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ if (magic != NBD_REP_MAGIC || option != NBD_OPT_SET_META_CONTEXT) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: invalid option reply magic or option");
+ return -1;
+ }
+ switch (reply) {
+ case NBD_REP_ACK: /* End of list of replies. */
+ if (len != 0) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: invalid option reply length");
+ return -1;
+ }
+ SET_NEXT_STATE (%^OPT_GO.START);
+ break;
+ case NBD_REP_META_CONTEXT: /* A context. */
+ if (len > maxpayload)
+ debug (h, "skipping too large meta context");
+ else if (len <= sizeof h->sbuf.or.payload.context.context) {
+ /* A valid reply has at least one byte in payload.context.str */
+ set_error (0, "handshake: NBD_REP_META_CONTEXT reply length too small");
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+ else {
len = be32toh (h->sbuf.or.option_reply.replylen);
meta_context = malloc (sizeof *meta_context);
@@ -225,20 +226,15 @@
h->meta_contexts = meta_context;
}
SET_NEXT_STATE (%PREPARE_FOR_REPLY);
+ break;
+ default:
+ /* Anything else is an error, ignore it */
+ /* XXX display any error message if NBD_REP_ERR_? */
+ debug (h, "handshake: unexpected error from "
+ "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
+ SET_NEXT_STATE (%^OPT_GO.START);
+ break;
}
return 0;
- NEWSTYLE.OPT_SET_META_CONTEXT.RECV_SKIP_PAYLOAD:
- switch (recv_into_rbuf (h)) {
- case -1: SET_NEXT_STATE (%.DEAD); return -1;
- case 0: SET_NEXT_STATE (%FINISH);
- /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
- }
- return 0;
-
- NEWSTYLE.OPT_SET_META_CONTEXT.FINISH:
- /* Jump to the next option. */
- SET_NEXT_STATE (%^OPT_GO.START);
- return 0;
-
} /* END STATE MACHINE */
--
2.20.1
4:53 p.m.
New subject: [libnbd PATCH 3/7] states: Allow large SET_CONTEXT_NAME replies
Although commit 5cc2547f rejects strings that are too long to send
over the protocol, it does not prevent the user from requesting a
context string longer than what we can store in sbuf. If the server
supports such a long context string, we end up blindly skipping the
server's reply as being oversized, and the user can't access that
context even though the server was happy to support it.
Of course, in practice, the only known contexts in existing servers
are not that long. But since we just recently enlarged struct
nbd_handle to support capture of the server's error messages on
structured replies, we might as well reuse that space to also capture
the full width of any possible context name.
---
lib/internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/internal.h b/lib/internal.h
index 6750938..5f19279 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -125,7 +125,7 @@ struct nbd_handle {
struct nbd_fixed_new_option_reply_info_export export;
struct {
struct nbd_fixed_new_option_reply_meta_context context;
- char str[64];
+ char str[NBD_MAX_STRING];
} __attribute__((packed)) context;
} payload;
} __attribute__((packed)) or;
--
2.20.1
4:54 p.m.
New subject: [libnbd PATCH 4/7] states: Rewrite NBD_REP_INFO parsing
Ensure that the payload was large enough, reject rather than ignore an
incorrect NBD_INFO_EXPORT size, and emit debug messages for the infos
that we are ignoring. Using a switch statement will also make it
easier for future patches to parse other NBD_INFO that we will
eventually care about.
---
generator/states-newstyle-opt-go.c | 40 ++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c
index 06bbaca..2ee739f 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -109,6 +109,7 @@
uint32_t reply;
uint32_t len;
const size_t maxpayload = sizeof h->sbuf.or.payload;
+ uint16_t info;
uint64_t exportsize;
uint16_t eflags;
@@ -131,20 +132,37 @@
SET_NEXT_STATE (%.READY);
return 0;
case NBD_REP_INFO:
- if (len <= maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */) {
- if (len >= sizeof h->sbuf.or.payload.export) {
- if (be16toh (h->sbuf.or.payload.export.info) == NBD_INFO_EXPORT) {
- exportsize = be64toh (h->sbuf.or.payload.export.exportsize);
- eflags = be16toh (h->sbuf.or.payload.export.eflags);
- if (nbd_internal_set_size_and_flags (h,
- exportsize, eflags) == -1) {
- SET_NEXT_STATE (%.DEAD);
- return -1;
- }
+ if (len > maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */) {
+ debug (h, "skipping large NBD_REP_INFO");
+ }
+ else if (len < sizeof h->sbuf.or.payload.export.info) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: NBD_REP_INFO reply length too small");
+ return -1;
+ }
+ else {
+ info = be16toh (h->sbuf.or.payload.export.info);
+ switch (info) {
+ case NBD_INFO_EXPORT:
+ if (len != sizeof h->sbuf.or.payload.export) {
+ SET_NEXT_STATE (%.DEAD);
+ set_error (0, "handshake: incorrect NBD_INFO_EXPORT option reply
length");
+ return -1;
}
+ exportsize = be64toh (h->sbuf.or.payload.export.exportsize);
+ eflags = be16toh (h->sbuf.or.payload.export.eflags);
+ if (nbd_internal_set_size_and_flags (h, exportsize, eflags) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+ break;
+ default:
+ /* XXX Handle other info types, like NBD_INFO_BLOCK_SIZE */
+ debug (h, "skipping unknown NBD_REP_INFO type %d",
+ be16toh (h->sbuf.or.payload.export.info));
+ break;
}
}
- /* ... else ignore the payload. */
/* Server is allowed to send any number of NBD_REP_INFO, read next one. */
h->rbuf = &h->sbuf;
h->rlen = sizeof (h->sbuf.or.option_reply);
--
2.20.1
4:54 p.m.
New subject: [libnbd PATCH 5/7] states: Factor out NBD_REP payload prep
Instead of repeating a check for valid reply headers in each sub-state
machine, let's have a common helper function do all the
work. Additionally, a central location will make it easier to
uniformly capture any NBD_REP_ERR message payloads.
---
generator/generator | 8 +--
generator/states-newstyle-opt-go.c | 40 +++----------
.../states-newstyle-opt-set-meta-context.c | 39 ++-----------
generator/states-newstyle-opt-starttls.c | 31 ++--------
.../states-newstyle-opt-structured-reply.c | 28 ++-------
generator/states-newstyle.c | 58 +++++++++++++++++++
6 files changed, 86 insertions(+), 118 deletions(-)
diff --git a/generator/generator b/generator/generator
index e3dd10f..a289741 100755
--- a/generator/generator
+++ b/generator/generator
@@ -373,8 +373,8 @@ and newstyle_opt_starttls_state_machine = [
State {
default_state with
- name = "SKIP_REPLY_PAYLOAD";
- comment = "Skip newstyle NBD_OPT_STARTTLS reply payload";
+ name = "RECV_REPLY_PAYLOAD";
+ comment = "Receive any newstyle NBD_OPT_STARTTLS reply payload";
external_events = [ NotifyRead, "" ];
};
@@ -425,8 +425,8 @@ and newstyle_opt_structured_reply_state_machine = [
State {
default_state with
- name = "SKIP_REPLY_PAYLOAD";
- comment = "Skip newstyle NBD_OPT_STRUCTURED_REPLY reply payload";
+ name = "RECV_REPLY_PAYLOAD";
+ comment = "Receive any newstyle NBD_OPT_STRUCTURED_REPLY reply payload";
external_events = [ NotifyRead, "" ];
};
diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c
index 2ee739f..3458f09 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -77,21 +77,13 @@
return 0;
NEWSTYLE.OPT_GO.RECV_REPLY:
- uint32_t len;
- const size_t maxpayload = sizeof h->sbuf.or.payload;
-
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- /* Read the following payload if it is short enough to fit in the
- * static buffer. If it's too long, skip it.
- */
- len = be32toh (h->sbuf.or.option_reply.replylen);
- if (len <= maxpayload)
- h->rbuf = &h->sbuf.or.payload;
- else
- h->rbuf = NULL;
- h->rlen = len;
+ if (prepare_for_reply_payload (h, NBD_OPT_GO) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
@@ -104,8 +96,6 @@
return 0;
NEWSTYLE.OPT_GO.CHECK_REPLY:
- uint64_t magic;
- uint32_t option;
uint32_t reply;
uint32_t len;
const size_t maxpayload = sizeof h->sbuf.or.payload;
@@ -113,34 +103,18 @@
uint64_t exportsize;
uint16_t eflags;
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
reply = be32toh (h->sbuf.or.option_reply.reply);
len = be32toh (h->sbuf.or.option_reply.replylen);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_GO) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
+
switch (reply) {
case NBD_REP_ACK:
- if (len != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
SET_NEXT_STATE (%.READY);
return 0;
case NBD_REP_INFO:
- if (len > maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */) {
+ if (len > maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */)
debug (h, "skipping large NBD_REP_INFO");
- }
- else if (len < sizeof h->sbuf.or.payload.export.info) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: NBD_REP_INFO reply length too small");
- return -1;
- }
else {
+ assert (len >= sizeof h->sbuf.or.payload.export.info);
info = be16toh (h->sbuf.or.payload.export.info);
switch (info) {
case NBD_INFO_EXPORT:
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index feaa295..94120e2 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -139,21 +139,13 @@
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY:
- uint32_t len;
- const uint32_t maxpayload = sizeof h->sbuf.or.payload.context;
-
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- /* Read the following payload if it is short enough to fit in the
- * static buffer. If it's too long, skip it.
- */
- len = be32toh (h->sbuf.or.option_reply.replylen);
- if (len <= maxpayload)
- h->rbuf = &h->sbuf.or.payload;
- else
- h->rbuf = NULL;
- h->rlen = len;
+ if (prepare_for_reply_payload (h, NBD_OPT_SET_META_CONTEXT) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
@@ -166,43 +158,22 @@
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.CHECK_REPLY:
- uint64_t magic;
- uint32_t option;
uint32_t reply;
uint32_t len;
const size_t maxpayload = sizeof h->sbuf.or.payload.context;
struct meta_context *meta_context;
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
reply = be32toh (h->sbuf.or.option_reply.reply);
len = be32toh (h->sbuf.or.option_reply.replylen);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_SET_META_CONTEXT) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
switch (reply) {
case NBD_REP_ACK: /* End of list of replies. */
- if (len != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
SET_NEXT_STATE (%^OPT_GO.START);
break;
case NBD_REP_META_CONTEXT: /* A context. */
if (len > maxpayload)
debug (h, "skipping too large meta context");
- else if (len <= sizeof h->sbuf.or.payload.context.context) {
- /* A valid reply has at least one byte in payload.context.str */
- set_error (0, "handshake: NBD_REP_META_CONTEXT reply length too small");
- SET_NEXT_STATE (%.DEAD);
- return -1;
- }
else {
- len = be32toh (h->sbuf.or.option_reply.replylen);
-
+ assert (len > sizeof h->sbuf.or.payload.context.context.context_id);
meta_context = malloc (sizeof *meta_context);
if (meta_context == NULL) {
set_error (errno, "malloc");
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index c5eba91..e994ffd 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -45,20 +45,18 @@
return 0;
NEWSTYLE.OPT_STARTTLS.RECV_REPLY:
- uint32_t len;
-
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- /* Discard the payload if there is one. */
- len = be32toh (h->sbuf.or.option_reply.replylen);
- h->rbuf = NULL;
- h->rlen = len;
- SET_NEXT_STATE (%SKIP_REPLY_PAYLOAD);
+ if (prepare_for_reply_payload (h, NBD_OPT_STARTTLS) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+ SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
- NEWSTYLE.OPT_STARTTLS.SKIP_REPLY_PAYLOAD:
+ NEWSTYLE.OPT_STARTTLS.RECV_REPLY_PAYLOAD:
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0: SET_NEXT_STATE (%CHECK_REPLY);
@@ -66,29 +64,12 @@
return 0;
NEWSTYLE.OPT_STARTTLS.CHECK_REPLY:
- uint64_t magic;
- uint32_t option;
uint32_t reply;
- uint32_t len;
struct socket *new_sock;
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
reply = be32toh (h->sbuf.or.option_reply.reply);
- len = be32toh (h->sbuf.or.option_reply.replylen);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_STARTTLS) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
switch (reply) {
case NBD_REP_ACK:
- if (len != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
-
new_sock = nbd_internal_crypto_create_session (h, h->sock);
if (new_sock == NULL) {
SET_NEXT_STATE (%.DEAD);
diff --git a/generator/states-newstyle-opt-structured-reply.c
b/generator/states-newstyle-opt-structured-reply.c
index 36f9eb3..d755411 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -39,20 +39,18 @@
return 0;
NEWSTYLE.OPT_STRUCTURED_REPLY.RECV_REPLY:
- uint32_t len;
-
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0:
- /* Discard the payload if there is one. */
- len = be32toh (h->sbuf.or.option_reply.replylen);
- h->rbuf = NULL;
- h->rlen = len;
- SET_NEXT_STATE (%SKIP_REPLY_PAYLOAD);
+ if (prepare_for_reply_payload (h, NBD_OPT_STRUCTURED_REPLY) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+ SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
}
return 0;
- NEWSTYLE.OPT_STRUCTURED_REPLY.SKIP_REPLY_PAYLOAD:
+ NEWSTYLE.OPT_STRUCTURED_REPLY.RECV_REPLY_PAYLOAD:
switch (recv_into_rbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return -1;
case 0: SET_NEXT_STATE (%CHECK_REPLY);
@@ -60,25 +58,11 @@
return 0;
NEWSTYLE.OPT_STRUCTURED_REPLY.CHECK_REPLY:
- uint64_t magic;
- uint32_t option;
uint32_t reply;
- magic = be64toh (h->sbuf.or.option_reply.magic);
- option = be32toh (h->sbuf.or.option_reply.option);
reply = be32toh (h->sbuf.or.option_reply.reply);
- if (magic != NBD_REP_MAGIC || option != NBD_OPT_STRUCTURED_REPLY) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply magic or option");
- return -1;
- }
switch (reply) {
case NBD_REP_ACK:
- if (h->sbuf.or.option_reply.replylen != 0) {
- SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: invalid option reply length");
- return -1;
- }
debug (h, "negotiated structured replies on this connection");
h->structured_replies = true;
break;
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index 6d81ab8..e86282b 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -16,6 +16,64 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
+#include "internal.h"
+
+/* Common code for parsing a reply to NBD_OPT_*. */
+static int
+prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt)
+{
+ const size_t maxpayload = sizeof h->sbuf.or.payload;
+ uint64_t magic;
+ uint32_t option;
+ uint32_t reply;
+ uint32_t len;
+
+ magic = be64toh (h->sbuf.or.option_reply.magic);
+ option = be32toh (h->sbuf.or.option_reply.option);
+ reply = be32toh (h->sbuf.or.option_reply.reply);
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ if (magic != NBD_REP_MAGIC || option != opt) {
+ set_error (0, "handshake: invalid option reply magic or option");
+ return -1;
+ }
+
+ /* Validate lengths that the state machine depends on. */
+ switch (reply) {
+ case NBD_REP_ACK:
+ if (len != 0) {
+ set_error (0, "handshake: invalid NBD_REP_ACK option reply length");
+ return -1;
+ }
+ break;
+ case NBD_REP_INFO:
+ /* Can't enforce an upper bound, thanks to unknown INFOs */
+ if (len < sizeof h->sbuf.or.payload.export.info) {
+ set_error (0, "handshake: NBD_REP_INFO reply length too small");
+ return -1;
+ }
+ break;
+ case NBD_REP_META_CONTEXT:
+ if (len <= sizeof h->sbuf.or.payload.context.context.context_id ||
+ len > sizeof h->sbuf.or.payload.context.context) {
+ set_error (0, "handshake: invalid NBD_REP_META_CONTEXT reply length");
+ return -1;
+ }
+ break;
+ }
+
+ /* Read the following payload if it is short enough to fit in the
+ * static buffer. If it's too long, skip it.
+ */
+ /* XXX Move to DEAD if len > MAX_REQUEST_SIZE */
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ if (len <= maxpayload)
+ h->rbuf = &h->sbuf.or.payload;
+ else
+ h->rbuf = NULL;
+ h->rlen = len;
+ return 0;
+}
+
/* State machine for parsing the fixed newstyle handshake. */
/* STATE MACHINE */ {
--
2.20.1
5:31 p.m.
New subject: [libnbd PATCH 5/7] states: Factor out NBD_REP payload prep
On 6/14/19 4:54 PM, Eric Blake wrote:
Instead of repeating a check for valid reply headers in each
sub-state
machine, let's have a common helper function do all the
work. Additionally, a central location will make it easier to
uniformly capture any NBD_REP_ERR message payloads.
---
generator/generator | 8 +--
generator/states-newstyle-opt-go.c | 40 +++----------
.../states-newstyle-opt-set-meta-context.c | 39 ++-----------
generator/states-newstyle-opt-starttls.c | 31 ++--------
.../states-newstyle-opt-structured-reply.c | 28 ++-------
generator/states-newstyle.c | 58 +++++++++++++++++++
6 files changed, 86 insertions(+), 118 deletions(-)
Aargh - I need to finish running 'make check' before posting. This one
breaks tests/meta-base-allocation unless:
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -139,21 +139,13 @@
return 0;
NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY:
- uint32_t len;
- const uint32_t maxpayload = sizeof h->sbuf.or.payload.context;
-
switch (recv_into_rbuf (h)) {
+++ b/generator/states-newstyle.c
@@ -16,6 +16,64 @@
+ case NBD_REP_META_CONTEXT:
+ if (len <= sizeof h->sbuf.or.payload.context.context.context_id ||
+ len > sizeof h->sbuf.or.payload.context.context) {
I fix this to be:
if (len <= sizeof h->sbuf.or.payload.context.context ||
len > sizeof h->sbuf.or.payload.context) {
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
4:54 p.m.
New subject: [libnbd PATCH 6/7] states: Give up on oversized reply length
Any server that sends a reply to NBD_OPT_* or NBD_CMD_* longer than
the maximum length we are willing to use for NBD_CMD_{READ,WRITE} is
probably broken; it is not worth waiting for read() to wait for that
many bytes to arrive from the server to bring the connection back into
sync for the next operation, so just declare it dead.
It may be possible to provoke nbdkit into being such a server when
replying to NBD_CMD_BLOCK_STATUS on a disk that reports alternating
extents, but if so, we should patch nbdkit to truncate before sending
that large of a chunk.
---
generator/states-newstyle.c | 7 +++++--
generator/states-reply-structured.c | 13 +++++++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index e86282b..369df0f 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -64,9 +64,12 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt)
/* Read the following payload if it is short enough to fit in the
* static buffer. If it's too long, skip it.
*/
- /* XXX Move to DEAD if len > MAX_REQUEST_SIZE */
len = be32toh (h->sbuf.or.option_reply.replylen);
- if (len <= maxpayload)
+ if (len > MAX_REQUEST_SIZE) {
+ set_error (0, "handshake: invalid option reply length");
+ return -1;
+ }
+ else if (len <= maxpayload)
h->rbuf = &h->sbuf.or.payload;
else
h->rbuf = NULL;
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 694c282..06eedb4 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -68,6 +68,19 @@
return -1;
}
+ /* Reject a server that replies with too much information, but don't
+ * reject a single structured reply to NBD_CMD_READ on the largest
+ * size we were willing to send. The most likely culprit is a server
+ * that replies with block status with way too many extents, but any
+ * oversized reply is going to take long enough to resync that it is
+ * not worth keeping the connection alive.
+ */
+ if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.sr.payload.offset_data) {
+ set_error (0, "invalid server reply length");
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+
if (NBD_REPLY_TYPE_IS_ERR (type)) {
if (length < sizeof h->sbuf.sr.payload.error) {
SET_NEXT_STATE (%.DEAD);
--
2.20.1
3:14 p.m.
New subject: [libnbd PATCH 6/7] states: Give up on oversized reply length
On 6/14/19 4:54 PM, Eric Blake wrote:
Any server that sends a reply to NBD_OPT_* or NBD_CMD_* longer than
the maximum length we are willing to use for NBD_CMD_{READ,WRITE} is
probably broken; it is not worth waiting for read() to wait for that
many bytes to arrive from the server to bring the connection back into
sync for the next operation, so just declare it dead.
It may be possible to provoke nbdkit into being such a server when
replying to NBD_CMD_BLOCK_STATUS on a disk that reports alternating
extents, but if so, we should patch nbdkit to truncate before sending
that large of a chunk.
In fact, after an hour of fiddling around, I proved it was possible, and
have since posted/pushed the corresponding nbdkit fix.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
4:54 p.m.
New subject: [libnbd PATCH 7/7] states: Capture NBD_REP_ERR message
Similar to the earlier patch for structured reply errors, it's worth
logging (at least through debug) any error messages that the server
sends alongside NBD_REP_ERR. And given our earlier changes, it
doesn't really impact the size of struct nbd_handle.
---
generator/states-newstyle-opt-go.c | 5 ++--
.../states-newstyle-opt-set-meta-context.c | 6 +++-
generator/states-newstyle-opt-starttls.c | 8 ++---
.../states-newstyle-opt-structured-reply.c | 6 +++-
generator/states-newstyle.c | 29 +++++++++++++++++++
lib/internal.h | 1 +
6 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c
index 3458f09..91a85ef 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -147,9 +147,10 @@
SET_NEXT_STATE (%^OPT_EXPORT_NAME.START);
return 0;
default:
+ if (handle_reply_error (h) == 0)
+ set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%" PRIx32,
+ reply);
SET_NEXT_STATE (%.DEAD);
- set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%" PRIx32,
- reply);
return -1;
}
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index 94120e2..a00a411 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -200,7 +200,11 @@
break;
default:
/* Anything else is an error, ignore it */
- /* XXX display any error message if NBD_REP_ERR_? */
+ if (handle_reply_error (h) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+
debug (h, "handshake: unexpected error from "
"NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
SET_NEXT_STATE (%^OPT_GO.START);
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index e994ffd..61f254f 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -83,9 +83,10 @@
return 0;
default:
- if (!NBD_REP_IS_ERR (reply))
- debug (h,
- "server is confused by NBD_OPT_STARTTLS, continuing anyway");
+ if (handle_reply_error (h) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
/* Server refused to upgrade to TLS. If h->tls is not require (2)
* then we can continue unencrypted.
@@ -100,7 +101,6 @@
debug (h,
"server refused TLS (%s), continuing with unencrypted connection",
reply == NBD_REP_ERR_POLICY ? "policy" : "not
supported");
- /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
return 0;
}
diff --git a/generator/states-newstyle-opt-structured-reply.c
b/generator/states-newstyle-opt-structured-reply.c
index d755411..65d5958 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -67,7 +67,11 @@
h->structured_replies = true;
break;
default:
- /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
+ if (handle_reply_error (h) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return -1;
+ }
+
debug (h, "structured replies are not supported by this server");
h->structured_replies = false;
break;
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index 369df0f..8a4eec6 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -77,6 +77,35 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt)
return 0;
}
+/* Check an unexpected server reply. If it is an error, log any
+ * message from the server and return 0; otherwise, return -1.
+ */
+static int
+handle_reply_error (struct nbd_handle *h)
+{
+ uint32_t len;
+ uint32_t reply;
+
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ reply = be32toh (h->sbuf.or.option_reply.reply);
+ if (!NBD_REP_IS_ERR (reply)) {
+ set_error (0, "handshake: unexpected option reply type %d", reply);
+ return -1;
+ }
+
+ assert (NBD_MAX_STRING < sizeof h->sbuf.or.payload);
+ if (len > NBD_MAX_STRING) {
+ set_error (0, "handshake: option error string too long");
+ return -1;
+ }
+
+ if (len > 0)
+ debug (h, "handshake: server error message: %.*s", (int) len,
+ h->sbuf.or.payload.err_msg);
+
+ return 0;
+}
+
/* State machine for parsing the fixed newstyle handshake. */
/* STATE MACHINE */ {
diff --git a/lib/internal.h b/lib/internal.h
index 5f19279..9acc1b4 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -127,6 +127,7 @@ struct nbd_handle {
struct nbd_fixed_new_option_reply_meta_context context;
char str[NBD_MAX_STRING];
} __attribute__((packed)) context;
+ char err_msg[NBD_MAX_STRING];
} payload;
} __attribute__((packed)) or;
struct nbd_export_name_option_reply export_name_reply;
--
2.20.1
5 p.m.
New subject: [libnbd PATCH 7/7] states: Capture NBD_REP_ERR message
On 6/14/19 4:54 PM, Eric Blake wrote:
Similar to the earlier patch for structured reply errors, it's
worth
logging (at least through debug) any error messages that the server
sends alongside NBD_REP_ERR. And given our earlier changes, it
doesn't really impact the size of struct nbd_handle.
---
+++ b/generator/states-newstyle.c
@@ -77,6 +77,35 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt)
return 0;
}
+/* Check an unexpected server reply. If it is an error, log any
+ * message from the server and return 0; otherwise, return -1.
+ */
+static int
+handle_reply_error (struct nbd_handle *h)
+{
+ uint32_t len;
+ uint32_t reply;
+
+ len = be32toh (h->sbuf.or.option_reply.replylen);
+ reply = be32toh (h->sbuf.or.option_reply.reply);
+ if (!NBD_REP_IS_ERR (reply)) {
+ set_error (0, "handshake: unexpected option reply type %d", reply);
+ return -1;
+ }
+
+ assert (NBD_MAX_STRING < sizeof h->sbuf.or.payload);
Missing an addition of #include <assert.h>.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
7:47 a.m.
New subject: [libnbd PATCH 7/7] states: Capture NBD_REP_ERR message
This patch series, plus the two fixes you noted in replies, looks fine
to me, so ACK series.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
2019
days inactive
2022
days old
11 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones