Libguestfs - Sep 2019 - Re: [nbdkit PATCH 4/4] server: Fix OPT_GO on different export than SET_META_CONTEXT

The first one is the nastiest - it is an assertion failure caused
by a spec-compliant client and introduced by our security fix
that was released in 1.14.1.

Eric Blake (4):
  server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO
  server: Fix back-to-back SET_META_CONTEXT
  server: Forbid NUL in export and context names
  server: Fix OPT_GO on different export than SET_META_CONTEXT

 server/internal.h                    |  5 ++-
 server/backend.c                     | 13 ++++++
 server/connections.c                 |  2 +-
 server/filters.c                     |  5 +--
 server/plugins.c                     |  4 --
 server/protocol-handshake-newstyle.c | 59 ++++++++++++++++++++++------
 6 files changed, 67 insertions(+), 21 deletions(-)

-- 
2.21.0

Most known NBD clients do not bother with NBD_OPT_INFO (except for
clients like 'qemu-nbd --list' that don't ever intend to connect),
but
go straight to NBD_OPT_GO.  However, it's not too hard to hack up qemu
to add in an extra client step (whether info on the same name, or more
interestingly, info on a different name), as a patch against qemu
commit 6f214b30445:

| diff --git i/nbd/client.c w/nbd/client.c
| index f6733962b49b..425292ac5ea9 100644
| --- i/nbd/client.c
| +++ w/nbd/client.c
| @@ -1038,6 +1038,14 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
|           * TLS).  If it is not available, fall back to
|           * NBD_OPT_LIST for nicer error messages about a missing
|           * export, then use NBD_OPT_EXPORT_NAME.  */
| +        if (getenv ("HACK"))
| +            info->name[0]++;
| +        result = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, info, errp);
| +        if (getenv ("HACK"))
| +            info->name[0]--;
| +        if (result < 0) {
| +            return -EINVAL;
| +        }
|          result = nbd_opt_info_or_go(ioc, NBD_OPT_GO, info, errp);
|          if (result < 0) {
|              return -EINVAL;

This works just fine in 1.14.0, where we call .open only once (so the
INFO and GO repeat calls into the same plugin handle), but in 1.14.1
it regressed into causing an assertion failure: we are now calling
.open a second time on a connection that is already opened.

$ nbdkit -rfv null &
$ hacked-qemu-io -f raw -r nbd://localhost -c quit
...
nbdkit: null[1]: debug: null: open readonly=1
nbdkit: backend.c:179: backend_open: Assertion `h->handle == NULL'
failed.

Worse, on the mainline development, we have recently made it possible
for plugins to actively report different information for different
export names; for example, a plugin may choose to report different
answers for .can_write on export A than for export B; but if we share
cached handles, then an NBD_OPT_INFO on one export prevents correct
answers for NBD_OPT_GO on the second export name.  (The HACK envvar in
my qemu modifications can be used to demonstrate cross-name requests,
which are even less likely in a real client).

The solution is to call .close after NBD_OPT_INFO, coupled with enough
glue logic to reset cached connection handles back to the state
expected by .open.  This in turn means factoring out another backend_*
function, but also gives us an opportunity to change
backend_set_handle to no longer accept NULL.

The assertion failure is, to some extent, a possible denial of service
attack (one client can force nbdkit to exit by merely sending OPT_INFO
before OPT_GO, preventing the next client from connecting), although
this is mitigated by using TLS to weed out untrusted clients.  Still,
the fact that we introduced a potential DoS attack while trying to fix
a traffic amplification security bug is not very nice.

Sadly, as there are no known clients that easily trigger this mode of
operation (OPT_INFO before OPT_GO), there is no easy way to cover this
via a testsuite addition.  I may end up hacking something into libnbd.

Fixes: c05686f957
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/internal.h                    |  4 +++-
 server/backend.c                     | 13 +++++++++++++
 server/connections.c                 |  2 +-
 server/filters.c                     |  5 +----
 server/plugins.c                     |  4 ----
 server/protocol-handshake-newstyle.c |  6 ++++++
 6 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index c31bb340..da4fae19 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -334,9 +334,11 @@ extern int backend_open (struct backend *b, struct
connection *conn,
   __attribute__((__nonnull__ (1, 2)));
 extern int backend_prepare (struct backend *b, struct connection *conn)
   __attribute__((__nonnull__ (1, 2)));
+extern void backend_close (struct backend *b, struct connection *conn)
+  __attribute__((__nonnull__ (1, 2)));
 extern void backend_set_handle (struct backend *b, struct connection *conn,
                                 void *handle)
-  __attribute__((__nonnull__ (1, 2 /* not 3 */)));
+  __attribute__((__nonnull__ (1, 2, 3)));
 extern bool backend_valid_range (struct backend *b, struct connection *conn,
                                  uint64_t offset, uint32_t count)
   __attribute__((__nonnull__ (1, 2)));
diff --git a/server/backend.c b/server/backend.c
index 3b213bfb..64dbf7db 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -201,10 +201,23 @@ backend_prepare (struct backend *b, struct connection
*conn)
   return b->prepare (b, conn, h->can_write == 0);
 }

+void
+backend_close (struct backend *b, struct connection *conn)
+{
+  struct b_conn_handle *h = &conn->handles[b->i];
+
+  debug ("%s: close", b->name);
+
+  b->close (b, conn);
+  memset (h, -1, sizeof *h);
+  h->handle = NULL;
+}
+
 void
 backend_set_handle (struct backend *b, struct connection *conn, void *handle)
 {
   assert (b->i < conn->nr_handles);
+  assert (conn->handles[b->i].handle == NULL);
   conn->handles[b->i].handle = handle;
 }

diff --git a/server/connections.c b/server/connections.c
index 819f7b86..3c4296ea 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -369,7 +369,7 @@ free_connection (struct connection *conn)
    */
   if (!quit && connection_get_handle (conn, 0)) {
     lock_request (conn);
-    backend->close (backend, conn);
+    backend_close (backend, conn);
     unlock_request (conn);
   }

diff --git a/server/filters.c b/server/filters.c
index 1ee62829..1091c2dd 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -225,12 +225,9 @@ filter_close (struct backend *b, struct connection *conn)
   struct backend_filter *f = container_of (b, struct backend_filter, backend);
   void *handle = connection_get_handle (conn, b->i);

-  debug ("%s: close", b->name);
-
   if (handle && f->filter.close)
     f->filter.close (handle);
-  backend_set_handle (b, conn, NULL);
-  b->next->close (b->next, conn);
+  backend_close (b->next, conn);
 }

 /* The next_functions structure contains pointers to backend
diff --git a/server/plugins.c b/server/plugins.c
index 9b44c548..727fb0e0 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -273,12 +273,8 @@ plugin_close (struct backend *b, struct connection *conn)

   assert (connection_get_handle (conn, 0));

-  debug ("close");
-
   if (p->plugin.close)
     p->plugin.close (connection_get_handle (conn, 0));
-
-  backend_set_handle (b, conn, NULL);
 }

 static int64_t
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 87e0bcd7..06fc53ad 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -469,6 +469,12 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
       if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
         return -1;

+      if (option == NBD_OPT_INFO) {
+        if (backend->finalize (backend, conn) == -1)
+          return -1;
+        backend_close (backend, conn);
+      }
+
       break;

     case NBD_OPT_STRUCTURED_REPLY:
-- 
2.21.0

The NBD spec says that if a client requests a 1-query SET_META_CONTEXT
for context "base:allocation", then changes its mind and requests a
1-query SET_META_CONTEXT for "other:context", only the final query
matters; in such a case, since we did not reply with a context the
second time, the client must not call NBD_CMD_BLOCK_STATUS, and the
server should fail it with EINVAL.  If the client actually wants two
contexts, it must request them in a 2-query SET_META_CONTEXT.
However, our code didn't reset the boolean between two uses of the
option, so we were not catching an invalid clients that requests block
status in spite of their second SET_META_CONTEXT.

Note that there are no known clients in the wild that can actually
perform this secondary SET_META_CONTEXT request; this was found by
inspection.  As nbdkit does not crash in this situation, I don't see
the point in hacking libnbd to make it possible to become such a
client.

Fixes: 26455d45
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/protocol-handshake-newstyle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 06fc53ad..38978c67 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -566,10 +566,10 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
         debug ("newstyle negotiation: %s: %s count: %d", optname,
                option == NBD_OPT_LIST_META_CONTEXT ? "query" :
"set",
                nr_queries);
+        if (option == NBD_OPT_SET_META_CONTEXT)
+          conn->meta_context_base_allocation = false;
         if (nr_queries == 0) {
-          if (option == NBD_OPT_SET_META_CONTEXT)
-            conn->meta_context_base_allocation = false;
-          else /* LIST */ {
+          if (option == NBD_OPT_LIST_META_CONTEXT) {
             if (send_newstyle_option_reply_meta_context
                 (conn, option, NBD_REP_META_CONTEXT,
                  0, "base:allocation") == -1)
-- 
2.21.0

A client that requests export or metacontext "a\0b" is not
spec-compliant (NBD strings must be UTF-8 with no embedded NUL).
What's more, our new nbdkit_export_name() and reflection plugin means
that a plugin serving content specific to export "a" may be exposing
something different from whatever the client was attempting to see.
Rather than changing our interface to allow embedded NUL, it's better
to just reject such a client as buggy.  This also means checking the
export name passed to NBD_OPT_SET_META_CONTEXT, rather than completely
ignoring it (although the next patch will further tweak that for a
different spec compliance reason).

It also didn't help that we were inconsistent between malloc/strcpy
vs. malloc/memcpy/set NUL.

I don't know of any existing NBD client that makes it easy to send
such an invalid export name, nor do I see any reason to hack libnbd
into providing that non-compliant behavior.

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

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 38978c67..785944eb 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -277,12 +277,15 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
       debug ("newstyle negotiation: %s: client requested export
'%s'",
              name_of_nbd_opt (option), data);
       free (conn->exportname);
-      conn->exportname = malloc (optlen+1);
+      conn->exportname = strndup (data, optlen);
       if (conn->exportname == NULL) {
-        nbdkit_error ("malloc: %m");
+        nbdkit_error ("strndup: %m");
+        return -1;
+      }
+      if (strlen (conn->exportname) != optlen) {
+        nbdkit_error ("export name must not contain NUL bytes");
         return -1;
       }
-      strcpy (conn->exportname, data);

       /* We have to finish the handshake by sending handshake_finish. */
       if (finish_newstyle_options (conn, &exportsize) == -1)
@@ -420,13 +423,15 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
          * save it in the connection.
          */
         free (conn->exportname);
-        conn->exportname = malloc (exportnamelen+1);
+        conn->exportname = strndup (&data[4], exportnamelen);
         if (conn->exportname == NULL) {
           nbdkit_error ("malloc: %m");
           return -1;
         }
-        memcpy (conn->exportname, &data[4], exportnamelen);
-        conn->exportname[exportnamelen] = '\0';
+        if (strlen (conn->exportname) != exportnamelen) {
+          nbdkit_error ("export name must not contain NUL bytes");
+          return -1;
+        }
         debug ("newstyle negotiation: %s: client requested export
'%s'",
                optname, conn->exportname);

@@ -547,9 +552,15 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
           continue;
         }

-        /* Discard the export name. */
+        /* Discard the export name, after checking that it is valid. */
         memcpy (&exportnamelen, &data[0], 4);
         exportnamelen = be32toh (exportnamelen);
+        what = "checking export name";
+        if (exportnamelen > optlen-8 /* NB optlen >= 8, see above */)
+          goto opt_meta_invalid_option_len;
+        what = "export name must not contain NUL bytes";
+        if (strnlen (&data[4], exportnamelen) != exportnamelen)
+          goto opt_meta_invalid_option_len;
         opt_index = 4 + exportnamelen;

         /* Read the number of queries. */
-- 
2.21.0

The NBD spec says that if a client requests SET_META_CONTEXT for
exportA, but later requests NBD_OPT_GO/EXPORT_NAME for exportB, then
the server should reject NBD_CMD_BLOCK_STATUS requests (that is, the
context returned for exportA need not apply to exportB).  When we
originally added base:allocation, our argument was that we always
ignore export names, so it was easier to just treat any two export
names as being the same export, so no need to reset things.  But now
that we have nbdkit_export_name(), we are better off obeying the spec.

Note that there are no known clients in the wild that can actually
perform this cross-export-name request; this was found by inspection.
I also don't see the point in hacking up libnbd to become such a
client.

Fixes: 4f303e44
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/internal.h                    |  1 +
 server/protocol-handshake-newstyle.c | 24 ++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index da4fae19..0603a747 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -182,6 +182,7 @@ struct connection {
   size_t nr_handles;

   char *exportname;
+  uint32_t exportnamelen;
   uint32_t cflags;
   uint16_t eflags;
   bool using_tls;
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 785944eb..45a65487 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -282,6 +282,7 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
         nbdkit_error ("strndup: %m");
         return -1;
       }
+      conn->exportnamelen = optlen;
       if (strlen (conn->exportname) != optlen) {
         nbdkit_error ("export name must not contain NUL bytes");
         return -1;
@@ -420,14 +421,21 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
         }

         /* As with NBD_OPT_EXPORT_NAME we print the export name and
-         * save it in the connection.
+         * save it in the connection.  For NBD_OPT_GO, if an earlier
+         * NBD_OPT_SET_META_CONTEXT used an export name, it must match
+         * or else we drop the support for that context.
          */
+        if (option == NBD_OPT_GO &&
conn->meta_context_base_allocation &&
+            (exportnamelen != conn->exportnamelen ||
+             memcmp (conn->exportname, &data[4], exportnamelen) != 0))
+          conn->meta_context_base_allocation = false;
         free (conn->exportname);
         conn->exportname = strndup (&data[4], exportnamelen);
         if (conn->exportname == NULL) {
           nbdkit_error ("malloc: %m");
           return -1;
         }
+        conn->exportnamelen = exportnamelen;
         if (strlen (conn->exportname) != exportnamelen) {
           nbdkit_error ("export name must not contain NUL bytes");
           return -1;
@@ -552,7 +560,10 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
           continue;
         }

-        /* Discard the export name, after checking that it is valid. */
+        /* Remember the export name: the NBD spec says that if the client
+         * later uses NBD_OPT_GO on a different export, then the context
+         * returned here is not usable.
+         */
         memcpy (&exportnamelen, &data[0], 4);
         exportnamelen = be32toh (exportnamelen);
         what = "checking export name";
@@ -561,6 +572,15 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
         what = "export name must not contain NUL bytes";
         if (strnlen (&data[4], exportnamelen) != exportnamelen)
           goto opt_meta_invalid_option_len;
+        if (option == NBD_OPT_SET_META_CONTEXT) {
+          free (conn->exportname);
+          conn->exportname = strndup (&data[4], exportnamelen);
+          if (conn->exportname == NULL) {
+            nbdkit_error ("strndup: %m");
+            return -1;
+          }
+          conn->exportnamelen = exportnamelen;
+        }
         opt_index = 4 + exportnamelen;

         /* Read the number of queries. */
-- 
2.21.0

ACK series, thanks.

It seems worth raising the assert failure with Red Hat's Security team
since it is a viable DoS attack on nbdkit servers.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 9/18/19 10:01 PM, Eric Blake wrote:
> The NBD spec says that if a client requests SET_META_CONTEXT for
> exportA, but later requests NBD_OPT_GO/EXPORT_NAME for exportB, then
> the server should reject NBD_CMD_BLOCK_STATUS requests (that is, the
> context returned for exportA need not apply to exportB).  When we
> originally added base:allocation, our argument was that we always
> ignore export names, so it was easier to just treat any two export
> names as being the same export, so no need to reset things.  But now
> that we have nbdkit_export_name(), we are better off obeying the spec.
> 
> Note that there are no known clients in the wild that can actually
> perform this cross-export-name request; this was found by inspection.
> I also don't see the point in hacking up libnbd to become such a
> client.
> 
> Fixes: 4f303e44
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  server/internal.h                    |  1 +
>  server/protocol-handshake-newstyle.c | 24 ++++++++++++++++++++++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
This missed NBD_OPT_EXPORT_NAME, I'll see if I can factor out a common
helper function rather than duplicating code.  (Then again, a client
that requests SET_META_CONTEXT but doesn't use OPT_GO is an anomaly)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Hi Eric,

Thank you for cc'ing secalert@.

I'll forward this to the team.

Best regards.

On Thu Sep 19 00:01:27 2019, eblake@redhat.com wrote:
> Most known NBD clients do not bother with NBD_OPT_INFO (except for
> clients like 'qemu-nbd --list' that don't ever intend to
connect), but
> go straight to NBD_OPT_GO. However, it's not too hard to hack up qemu
> to add in an extra client step (whether info on the same name, or more
> interestingly, info on a different name), as a patch against qemu
> commit 6f214b30445:
>
> | diff --git i/nbd/client.c w/nbd/client.c
> | index f6733962b49b..425292ac5ea9 100644
> | --- i/nbd/client.c
> | +++ w/nbd/client.c
> | @@ -1038,6 +1038,14 @@ int nbd_receive_negotiate(AioContext
> *aio_context, QIOChannel *ioc,
> | * TLS). If it is not available, fall back to
> | * NBD_OPT_LIST for nicer error messages about a missing
> | * export, then use NBD_OPT_EXPORT_NAME. */
> | + if (getenv ("HACK"))
> | + info->name[0]++;
> | + result = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, info, errp);
> | + if (getenv ("HACK"))
> | + info->name[0]--;
> | + if (result < 0) {
> | + return -EINVAL;
> | + }
> | result = nbd_opt_info_or_go(ioc, NBD_OPT_GO, info, errp);
> | if (result < 0) {
> | return -EINVAL;
>
> This works just fine in 1.14.0, where we call .open only once (so the
> INFO and GO repeat calls into the same plugin handle), but in 1.14.1
> it regressed into causing an assertion failure: we are now calling
> .open a second time on a connection that is already opened.
>
> $ nbdkit -rfv null &
> $ hacked-qemu-io -f raw -r nbd://localhost -c quit
> ...
> nbdkit: null[1]: debug: null: open readonly=1
> nbdkit: backend.c:179: backend_open: Assertion `h->handle == NULL'
> failed.
>
> Worse, on the mainline development, we have recently made it possible
> for plugins to actively report different information for different
> export names; for example, a plugin may choose to report different
> answers for .can_write on export A than for export B; but if we share
> cached handles, then an NBD_OPT_INFO on one export prevents correct
> answers for NBD_OPT_GO on the second export name. (The HACK envvar in
> my qemu modifications can be used to demonstrate cross-name requests,
> which are even less likely in a real client).
>
> The solution is to call .close after NBD_OPT_INFO, coupled with enough
> glue logic to reset cached connection handles back to the state
> expected by .open. This in turn means factoring out another backend_*
> function, but also gives us an opportunity to change
> backend_set_handle to no longer accept NULL.
>
> The assertion failure is, to some extent, a possible denial of service
> attack (one client can force nbdkit to exit by merely sending OPT_INFO
> before OPT_GO, preventing the next client from connecting), although
> this is mitigated by using TLS to weed out untrusted clients. Still,
> the fact that we introduced a potential DoS attack while trying to fix
> a traffic amplification security bug is not very nice.
>
> Sadly, as there are no known clients that easily trigger this mode of
> operation (OPT_INFO before OPT_GO), there is no easy way to cover this
> via a testsuite addition. I may end up hacking something into libnbd.
>
> Fixes: c05686f957
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> server/internal.h | 4 +++-
> server/backend.c | 13 +++++++++++++
> server/connections.c | 2 +-
> server/filters.c | 5 +----
> server/plugins.c | 4 ----
> server/protocol-handshake-newstyle.c | 6 ++++++
> 6 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/server/internal.h b/server/internal.h
> index c31bb340..da4fae19 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -334,9 +334,11 @@ extern int backend_open (struct backend *b,
> struct connection *conn,
> __attribute__((__nonnull__ (1, 2)));
> extern int backend_prepare (struct backend *b, struct connection
> *conn)
> __attribute__((__nonnull__ (1, 2)));
> +extern void backend_close (struct backend *b, struct connection
> *conn)
> + __attribute__((__nonnull__ (1, 2)));
> extern void backend_set_handle (struct backend *b, struct connection
> *conn,
> void *handle)
> - __attribute__((__nonnull__ (1, 2 /* not 3 */)));
> + __attribute__((__nonnull__ (1, 2, 3)));
> extern bool backend_valid_range (struct backend *b, struct connection
> *conn,
> uint64_t offset, uint32_t count)
> __attribute__((__nonnull__ (1, 2)));
> diff --git a/server/backend.c b/server/backend.c
> index 3b213bfb..64dbf7db 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -201,10 +201,23 @@ backend_prepare (struct backend *b, struct
> connection *conn)
> return b->prepare (b, conn, h->can_write == 0);
> }
>
> +void
> +backend_close (struct backend *b, struct connection *conn)
> +{
> + struct b_conn_handle *h = &conn->handles[b->i];
> +
> + debug ("%s: close", b->name);
> +
> + b->close (b, conn);
> + memset (h, -1, sizeof *h);
> + h->handle = NULL;
> +}
> +
> void
> backend_set_handle (struct backend *b, struct connection *conn, void
> *handle)
> {
> assert (b->i < conn->nr_handles);
> + assert (conn->handles[b->i].handle == NULL);
> conn->handles[b->i].handle = handle;
> }
>
> diff --git a/server/connections.c b/server/connections.c
> index 819f7b86..3c4296ea 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -369,7 +369,7 @@ free_connection (struct connection *conn)
> */
> if (!quit && connection_get_handle (conn, 0)) {
> lock_request (conn);
> - backend->close (backend, conn);
> + backend_close (backend, conn);
> unlock_request (conn);
> }
>
> diff --git a/server/filters.c b/server/filters.c
> index 1ee62829..1091c2dd 100644
> --- a/server/filters.c
> +++ b/server/filters.c
> @@ -225,12 +225,9 @@ filter_close (struct backend *b, struct
> connection *conn)
> struct backend_filter *f = container_of (b, struct backend_filter,
> backend);
> void *handle = connection_get_handle (conn, b->i);
>
> - debug ("%s: close", b->name);
> -
> if (handle && f->filter.close)
> f->filter.close (handle);
> - backend_set_handle (b, conn, NULL);
> - b->next->close (b->next, conn);
> + backend_close (b->next, conn);
> }
>
> /* The next_functions structure contains pointers to backend
> diff --git a/server/plugins.c b/server/plugins.c
> index 9b44c548..727fb0e0 100644
> --- a/server/plugins.c
> +++ b/server/plugins.c
> @@ -273,12 +273,8 @@ plugin_close (struct backend *b, struct
> connection *conn)
>
> assert (connection_get_handle (conn, 0));
>
> - debug ("close");
> -
> if (p->plugin.close)
> p->plugin.close (connection_get_handle (conn, 0));
> -
> - backend_set_handle (b, conn, NULL);
> }
>
> static int64_t
> diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-
> handshake-newstyle.c
> index 87e0bcd7..06fc53ad 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -469,6 +469,12 @@ negotiate_handshake_newstyle_options (struct
> connection *conn)
> if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) => -1)
> return -1;
>
> + if (option == NBD_OPT_INFO) {
> + if (backend->finalize (backend, conn) == -1)
> + return -1;
> + backend_close (backend, conn);
> + }
> +
> break;
>
> case NBD_OPT_STRUCTURED_REPLY:

--
Pedro Sampaio - Red Hat Product Security
3635 1D27 6A05 6AC3 BC0B 30A4 52CF 575B E51B 20F4