Libguestfs - Sep 2020 - [nbdkit PATCH] server: Adjust limit on max NBD_OPT_* from client

Up to nbdkit 1.22, we never advertised large export lists, so a client
had no real reason to give more than 32 NBD_OPT_ commands before
finally selecting an export or quitting, and we were justified in
dropping indecisive chatty clients as being a waste of server
resources.  But now that we support .list_exports, it is reasonable
for a client (such as 'qemu-nbd --list' or 'nbdinfo --list') to
want
to call NBD_OPT_INFO and several NBD_OPT_LIST_META_CONTEXT commands
for every export returned in NBD_OPT_LIST.  We still want to avoid
clients that can tie us up in eternal handshaking, so let's reject a
second call to NBD_OPT_LIST; but once the first call is made, the
client now has a chance to get info on everything listed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/protocol-handshake-newstyle.c | 49 ++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 9c86639b..dedb7f9d 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -44,5 +44,8 @@
 #include "nbd-protocol.h"
 #include "protostrings.h"

-/* Maximum number of client options we allow before giving up. */
+/* Initial bound of client options we allow before giving up.
+ * However, a client that issues NBD_OPT_LIST is permitted to follow
+ * up with another round of options per export listed.
+ */
 #define MAX_NR_OPTIONS 32

 /* Receive newstyle options. */
@@ -78,12 +81,13 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply)
 /* Reply to NBD_OPT_LIST with the plugin's list of export names.
  */
 static int
-send_newstyle_option_reply_exportnames (uint32_t option)
+send_newstyle_option_reply_exportnames (uint32_t option, size_t *nr_options)
 {
   GET_CONN;
   struct nbd_fixed_new_option_reply fixed_new_option_reply;
-  size_t i;
+  size_t i, list_len;
   CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL;
+  int r;

   exps = nbdkit_exports_new ();
   if (exps == NULL)
@@ -91,7 +95,8 @@ send_newstyle_option_reply_exportnames (uint32_t option)
   if (backend_list_exports (top, read_only, exps) == -1)
     return send_newstyle_option_reply (option, NBD_REP_ERR_PLATFORM);

-  for (i = 0; i < nbdkit_exports_count (exps); i++) {
+  list_len = nbdkit_exports_count (exps);
+  for (i = 0; i < list_len; i++) {
     const struct nbdkit_export export = nbdkit_get_export (exps, i);
     size_t name_len = strlen (export.name);
     size_t desc_len = export.description ? strlen (export.description) : 0;
@@ -127,7 +132,11 @@ send_newstyle_option_reply_exportnames (uint32_t option)
     }
   }

-  return send_newstyle_option_reply (option, NBD_REP_ACK);
+  r = send_newstyle_option_reply (option, NBD_REP_ACK);
+  /* Allow additional per-export NBD_OPT_INFO and friends. */
+  if (r == 0)
+    *nr_options += MAX_NR_OPTIONS * list_len;
+  return r;
 }

 static int
@@ -326,6 +335,7 @@ negotiate_handshake_newstyle_options (void)
   GET_CONN;
   struct nbd_new_option new_option;
   size_t nr_options;
+  bool list_seen = false;
   uint64_t version;
   uint32_t option;
   uint32_t optlen;
@@ -334,7 +344,7 @@ negotiate_handshake_newstyle_options (void)
   uint64_t exportsize;
   struct backend *b;

-  for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
+  for (nr_options = MAX_NR_OPTIONS; nr_options > 0; --nr_options) {
     CLEANUP_FREE char *data = NULL;

     if (conn_recv_full (&new_option, sizeof new_option,
@@ -431,11 +441,22 @@ negotiate_handshake_newstyle_options (void)
         continue;
       }

-      /* Send back the exportname list. */
-      debug ("newstyle negotiation: %s: advertising exports",
-             name_of_nbd_opt (option));
-      if (send_newstyle_option_reply_exportnames (option) == -1)
-        return -1;
+      if (list_seen) {
+        debug ("newstyle negotiation: %s: export list already
advertised",
+               name_of_nbd_opt (option));
+        if (send_newstyle_option_reply (option, NBD_REP_ERR_INVALID)
+            == -1)
+          return -1;
+        continue;
+      }
+      else {
+        /* Send back the exportname list. */
+        debug ("newstyle negotiation: %s: advertising exports",
+               name_of_nbd_opt (option));
+        if (send_newstyle_option_reply_exportnames (option, &nr_options) ==
-1)
+          return -1;
+        list_seen = true;
+      }
       break;

     case NBD_OPT_STARTTLS:
@@ -826,8 +847,8 @@ negotiate_handshake_newstyle_options (void)
       break;
   }

-  if (nr_options >= MAX_NR_OPTIONS) {
-    nbdkit_error ("client exceeded maximum number of options (%d)",
-                  MAX_NR_OPTIONS);
+  if (nr_options == 0) {
+    nbdkit_error ("client spent too much time negotiating without
selecting "
+                  "an export");
     return -1;
   }

-- 
2.28.0

(I thought I had replied to this already, but I can't find the
reply now so maybe I didn't.)

Anyway, looks good, ACK.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW