thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH 0/7] state machine refactoring [Jun 2019]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2019-Jun-14 21:53 UTC

[Libguestfs] [libnbd PATCH 0/7] state machine refactoring

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

Eric Blake

2019-Jun-14 21:53 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:53 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:53 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:54 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:54 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:54 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 21:54 UTC

head link

[Libguestfs] [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

Eric Blake

2019-Jun-14 22:00 UTC

head link

Re: [Libguestfs] [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

Eric Blake

2019-Jun-14 22:31 UTC

head link

Re: [Libguestfs] [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

Eric Blake

2019-Jun-17 20:14 UTC

head link

Re: [Libguestfs] [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

Maybe Matching Threads

Search for more apparently analagous threads

Libguestfs - Jun 2019 - [libnbd PATCH 0/7] state machine refactoring

[Libguestfs] [libnbd PATCH 0/7] state machine refactoring

[Libguestfs] [libnbd PATCH 1/7] states: Log structured error messages, if any

[Libguestfs] [libnbd PATCH 2/7] states: Refactor SET_META_CONTEXT reply parsing

[Libguestfs] [libnbd PATCH 3/7] states: Allow large SET_CONTEXT_NAME replies

[Libguestfs] [libnbd PATCH 4/7] states: Rewrite NBD_REP_INFO parsing

[Libguestfs] [libnbd PATCH 5/7] states: Factor out NBD_REP payload prep

[Libguestfs] [libnbd PATCH 6/7] states: Give up on oversized reply length

[Libguestfs] [libnbd PATCH 7/7] states: Capture NBD_REP_ERR message

Re: [Libguestfs] [libnbd PATCH 7/7] states: Capture NBD_REP_ERR message

Re: [Libguestfs] [libnbd PATCH 5/7] states: Factor out NBD_REP payload prep

Re: [Libguestfs] [libnbd PATCH 6/7] states: Give up on oversized reply length

Maybe Matching Threads