Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 0/7] Spec compliance patches
Since the v1 series (0/4, at [1]), I've applied patches 1 and 2, rewritten patch 3 [Forbid NUL in export and context names] into patch 4 here, patch 4 there turned into patch 6 here, and everything else here is new. [1]https://www.redhat.com/archives/libguestfs/2019-September/msg00180.html I don't know if there is a handy reusable function for checking whether a string contains valid UTF-8 while still complying with our licensing, so for now I don't bother to check that. But fixing this meant I also proposed a qemu 4.2 patch on export name length: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg06925.html so part of the test I added only really covers nbdkit when using qemu 4.1, because otherwise qemu fails early without involving nbdkit. Eric Blake (7): server: Reject -e with too long of a name server: Expose -e in $exportname during --run server: Switch to fixed-length conn->exportname server: Improve check of string validity server: Allow longer NBD_OPT server: Fix OPT_GO on different export than SET_META_CONTEXT server: Better newstyle .open failure handling docs/nbdkit-captive.pod | 12 ++- server/internal.h | 4 +- server/Makefile.am | 1 + server/captive.c | 22 ++++- server/connections.c | 7 -- server/main.c | 6 ++ server/protocol-handshake-newstyle.c | 117 ++++++++++++++++++++------- server/protocol-handshake-oldstyle.c | 3 + tests/Makefile.am | 2 + tests/test-long-name.sh | 101 +++++++++++++++++++++++ 10 files changed, 231 insertions(+), 44 deletions(-) create mode 100755 tests/test-long-name.sh -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 1/7] server: Reject -e with too long of a name
The NBD protocol requires strings to be capped at 4k; we violated that if the client requests NBD_OPT_LIST but the command line provided too long of a string. The protocol also requires that strings be valid UTF-8, but for now, we are accepting any byte sequence. However, we still need another patch before the test is fully complete: qemu-nbd --list wants to send a valid NBD_OPT_INFO with length longer than we currently permit. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/main.c | 6 ++++ tests/Makefile.am | 2 ++ tests/test-long-name.sh | 65 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100755 tests/test-long-name.sh diff --git a/server/main.c b/server/main.c index 975716dc..56231497 100644 --- a/server/main.c +++ b/server/main.c @@ -51,6 +51,7 @@ #include <dlfcn.h> #include "internal.h" +#include "nbd-protocol.h" #include "options.h" #include "exit-with-parent.h" @@ -330,6 +331,11 @@ main (int argc, char *argv[]) case 'e': exportname = optarg; + if (strnlen (exportname, NBD_MAX_STRING + 1) > NBD_MAX_STRING) { + nbdkit_error ("export name too long"); + exit (EXIT_FAILURE); + } + /* TODO: Check that name is valid UTF-8? */ newstyle = true; break; diff --git a/tests/Makefile.am b/tests/Makefile.am index 7d254be9..046e219a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -79,6 +79,7 @@ EXTRA_DIST = \ test-linuxdisk.sh \ test-linuxdisk-copy-out.sh \ test-log.sh \ + test-long-name.sh \ test.lua \ test-memory-largest.sh \ test-memory-largest-for-qemu.sh \ @@ -198,6 +199,7 @@ TESTS += \ test-socket-activation \ test-foreground.sh \ test-debug-flags.sh \ + test-long-name.sh \ $(NULL) check_PROGRAMS += \ diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh new file mode 100755 index 00000000..214a5e7a --- /dev/null +++ b/tests/test-long-name.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +fail=0 + +# Test handling of NBD maximum string length of 4k. + +requires qemu-nbd --version + +name16=1234567812345678 +name64=$name16$name16$name16$name16 +name256=$name64$name64$name64$name64 +name1k=$name256$name256$name256$name256 +name4k=$name1k$name1k$name1k$name1k +almost4k=${name4k%8$name16} + +# Test behavior of -e: accept 4k max, reject longer +nbdkit -U - -e $name4k null --run true || fail=1 +nbdkit -U - -e a$name4k null --run true && fail=1 + +# The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0. +if ! qemu-nbd --help | grep -sq -- --list; then + echo "$0: skipping because qemu-nbd does not support the --list option" + exit 77 +fi + +# Test response to NBD_OPT_LIST +nbdkit -U - -e $almost4k null --run 'qemu-nbd --list -k $unixsocket' || fail=1 +# FIXME: Right now, we can't accept full 4k length - this should succeed +nbdkit -U - -e $name4k null --run 'qemu-nbd --list -k $unixsocket' && fail=1 + +exit $fail -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 2/7] server: Expose -e in $exportname during --run
Now that the export name can affect contents, we should let --run favor connecting to the preferred export name passed to nbdkit via the -e command line option. In addition to exposing $exportname, update the recently-added $uri to include the exportname, but leave the older $nbd alone. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-captive.pod | 12 ++++++++++-- server/captive.c | 22 ++++++++++++++++++---- tests/test-long-name.sh | 20 ++++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod index 0adf0a0b..9c7044f1 100644 --- a/docs/nbdkit-captive.pod +++ b/docs/nbdkit-captive.pod @@ -45,12 +45,14 @@ The following shell variables are available in the I<--run> argument: =item C<$uri> A URI that refers to the nbdkit port or socket in the preferred form -documented by the NBD project. +documented by the NBD project. If nbdkit was started with the B<-e> +option to set the preferred export name, this is included in the URI. =item C<$nbd> An older URL that refers to the nbdkit port or socket in a manner -specific to certain tools. +specific to certain tools. This form does not include an export name, +even if B<-e> was used. Note there is some magic here, since qemu and guestfish URLs have a different format, so nbdkit tries to guess which you are running. If @@ -64,6 +66,12 @@ If E<ne> "", the port number that nbdkit is listening on. If E<ne> "", the Unix domain socket that nbdkit is listening on. +=item C<$exportname> + +The default export name (which may be "") that nbdkit will advertise +in response to NBD_OPT_LIST. This comes from the B<-e> +(B<--exportname>) command line option. + =back I<--run> implies I<--foreground>. It is not possible, and probably diff --git a/server/captive.c b/server/captive.c index 90e42050..c4cec238 100644 --- a/server/captive.c +++ b/server/captive.c @@ -71,12 +71,26 @@ run_command (void) if (port) { fprintf (fp, "nbd://localhost:"); shell_quote (port, fp); + if (exportname) { + putc ('/', fp); + uri_quote (exportname, fp); + } } else if (unixsocket) { - fprintf (fp, "nbd+unix://\\?socket="); + fprintf (fp, "nbd+unix://"); + if (exportname) { + putc ('/', fp); + uri_quote (exportname, fp); + } + fprintf (fp, "\\?socket="); uri_quote (unixsocket, fp); } - fprintf (fp, "\n"); + putc ('\n', fp); + + /* Expose $exportname. */ + fprintf (fp, "exportname="); + shell_quote (exportname, fp); + putc ('\n', fp); /* Construct older $nbd "URL". Unfortunately guestfish and qemu take * different syntax, so try to guess which one we need. @@ -106,13 +120,13 @@ run_command (void) else abort (); } - fprintf (fp, "\n"); + putc ('\n', fp); /* Construct $port and $unixsocket. */ fprintf (fp, "port="); if (port) shell_quote (port, fp); - fprintf (fp, "\n"); + putc ('\n', fp); fprintf (fp, "unixsocket="); if (unixsocket) shell_quote (unixsocket, fp); diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh index 214a5e7a..7b0b43ad 100755 --- a/tests/test-long-name.sh +++ b/tests/test-long-name.sh @@ -51,6 +51,26 @@ almost4k=${name4k%8$name16} nbdkit -U - -e $name4k null --run true || fail=1 nbdkit -U - -e a$name4k null --run true && fail=1 +# Test that $exportname and $uri reflect the name +out=$(nbdkit -U - -e $name4k null --run 'echo $exportname') +if test "$name4k" != "$out"; then + echo "$0: \$exportname contains wrong contents" >&2 + fail=1 +fi +out=$(nbdkit -U - -e $name4k null --run 'echo $uri') +case $out in + nbd+unix:///$name4k\?socket=*) ;; + *) echo "$0: \$uri contains wrong contents" >&2 + fail=1 ;; +esac +pick_unused_port +out=$(nbdkit -i localhost -p $port -e $name4k null --run 'echo $uri') +case $out in + nbd://localhost:$port/$name4k) ;; + *) echo "$0: \$uri contains wrong contents" >&2 + fail=1 ;; +esac + # The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0. if ! qemu-nbd --help | grep -sq -- --list; then echo "$0: skipping because qemu-nbd does not support the --list option" -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 3/7] server: Switch to fixed-length conn->exportname
Since each connection already has a malloc()d struct conn, and the NBD protocol places a fixed length on the maximum string at 4k, it's not that much harder to just directly set aside 4k in the connection struct instead of allocating a name on the fly. In the common case of short names, it does mean that a connection now occupies slightly more unused heap space, but these days, 4k overhead is probably in the noise. Doing this makes it easier for an upcoming patch to refactor name validation without having to worry about an allocation failure, which in turn makes it easier to keep the connection alive even after failing NBD_OPT_GO (where an allocation failure would have to be handled differently than the diagnosis of an invalid client request). This also points out a spot where we need to enlarge MAX_OPTION_LENGTH, but that will be a separate patch. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/internal.h | 3 ++- server/Makefile.am | 1 + server/connections.c | 7 ------- server/protocol-handshake-newstyle.c | 17 +++++------------ 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/server/internal.h b/server/internal.h index fbabce69..e6a22f1a 100644 --- a/server/internal.h +++ b/server/internal.h @@ -43,6 +43,7 @@ #include "nbdkit-plugin.h" #include "nbdkit-filter.h" #include "cleanup.h" +#include "nbd-protocol.h" #ifdef __APPLE__ #define UNIX_PATH_MAX 104 @@ -198,7 +199,7 @@ struct connection { struct b_conn_handle *handles; size_t nr_handles; - char *exportname; + char exportname[NBD_MAX_STRING + 1]; uint32_t cflags; uint16_t eflags; bool using_tls; diff --git a/server/Makefile.am b/server/Makefile.am index a0417639..c846fc7a 100644 --- a/server/Makefile.am +++ b/server/Makefile.am @@ -134,6 +134,7 @@ test_public_SOURCES = \ test_public_CPPFLAGS = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/protocol \ -I$(top_srcdir)/common/utils \ $(NULL) test_public_CFLAGS = $(WARNINGS_CFLAGS) $(VALGRIND_CFLAGS) diff --git a/server/connections.c b/server/connections.c index eeda85d5..27cf202b 100644 --- a/server/connections.c +++ b/server/connections.c @@ -258,11 +258,6 @@ new_connection (int sockin, int sockout, int nworkers) conn->status_pipe[0] = conn->status_pipe[1] = -1; - conn->exportname = strdup (""); - if (conn->exportname == NULL) { - perror ("strdup"); - goto error; - } conn->handles = calloc (backend->i + 1, sizeof *conn->handles); if (conn->handles == NULL) { perror ("malloc"); @@ -335,7 +330,6 @@ new_connection (int sockin, int sockout, int nworkers) close (conn->status_pipe[0]); if (conn->status_pipe[1] >= 0) close (conn->status_pipe[1]); - free (conn->exportname); free (conn->handles); free (conn); return NULL; @@ -383,7 +377,6 @@ free_connection (struct connection *conn) pthread_mutex_destroy (&conn->status_lock); free (conn->handles); - free (conn->exportname); free (conn); } diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index d0fb4dd7..0cbcc3d5 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -277,12 +277,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) data[optlen] = '\0'; debug ("newstyle negotiation: %s: client requested export '%s'", name_of_nbd_opt (option), data); - free (conn->exportname); - conn->exportname = malloc (optlen+1); - if (conn->exportname == NULL) { - nbdkit_error ("malloc: %m"); - return -1; - } + assert (optlen < sizeof conn->exportname); strcpy (conn->exportname, data); /* We have to finish the handshake by sending handshake_finish. */ @@ -420,12 +415,10 @@ negotiate_handshake_newstyle_options (struct connection *conn) /* As with NBD_OPT_EXPORT_NAME we print the export name and * save it in the connection. */ - free (conn->exportname); - conn->exportname = malloc (exportnamelen+1); - if (conn->exportname == NULL) { - nbdkit_error ("malloc: %m"); - return -1; - } + /* FIXME: Our current MAX_OPTION_LENGTH prevents us from receiving + * an export name at the full NBD_MAX_STRING length. + */ + assert (exportnamelen < sizeof conn->exportname); memcpy (conn->exportname, &data[4], exportnamelen); conn->exportname[exportnamelen] = '\0'; debug ("newstyle negotiation: %s: client requested export '%s'", -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 4/7] server: Improve check of string validity
Factor out a common function for checking whether a client export name is valid, checking for length and no embedded NULs (in the future, we may also add a check for well-formed UTF-8). Checking for embedded NUL is important now that we allow plugins to alter content based on the client's request: if the client requested 'a\0b' (against the NBD protocol), they would be surprised if the reflection plugin only gave back 'a'. Rejecting bad strings is easier than altering our API to pass through lengths to preserve embedded NUL. Use the new functions to simplify existing code in NBD_OPT_EXPORT_NAME and NBD_OPT_GO, but also to validate the export name handed to NBD_OPT_SET_META_CONTEXT. No known existing client makes it easy to send 'a\0b' as an export or meta context name against the protocol, but well-place breakpoints in gdb on a client process allowed me to test this manually. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/protocol-handshake-newstyle.c | 62 ++++++++++++++++++++++------ tests/test-long-name.sh | 18 ++++++++ 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 0cbcc3d5..34958360 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -210,6 +210,43 @@ finish_newstyle_options (struct connection *conn, uint64_t *exportsize) return 0; } +static int +check_string (uint32_t option, char *buf, uint32_t len, uint32_t maxlen, + const char *name) +{ + if (len > NBD_MAX_STRING || len > maxlen) { + nbdkit_error ("%s: %s too long", name_of_nbd_opt (option), name); + return -1; + } + if (strnlen (buf, len) != len) { + nbdkit_error ("%s: %s may not include NUL bytes", + name_of_nbd_opt (option), name); + return -1; + } + /* TODO: Check for valid UTF-8? */ + return 0; +} + +/* Sub-function of negotiate_handshake_newstyle_options, to grab and + * validate an export name. + */ +static int +check_export_name (struct connection *conn, uint32_t option, char *buf, + uint32_t exportnamelen, uint32_t maxlen, bool save) +{ + if (check_string (option, buf, exportnamelen, maxlen, "export name") == -1) + return -1; + + assert (exportnamelen < sizeof conn->exportname); + if (save) { + memcpy (conn->exportname, buf, exportnamelen); + conn->exportname[exportnamelen] = '\0'; + } + debug ("newstyle negotiation: %s: client requested export '%.*s'", + name_of_nbd_opt (option), (int) exportnamelen, buf); + return 0; +} + static int negotiate_handshake_newstyle_options (struct connection *conn) { @@ -273,12 +310,8 @@ negotiate_handshake_newstyle_options (struct connection *conn) if (conn_recv_full (conn, data, optlen, "read: %s: %m", name_of_nbd_opt (option)) == -1) return -1; - /* Print the export name and save it in the connection. */ - data[optlen] = '\0'; - debug ("newstyle negotiation: %s: client requested export '%s'", - name_of_nbd_opt (option), data); - assert (optlen < sizeof conn->exportname); - strcpy (conn->exportname, data); + if (check_export_name (conn, option, data, optlen, optlen, true) == -1) + return -1; /* We have to finish the handshake by sending handshake_finish. */ if (finish_newstyle_options (conn, &exportsize) == -1) @@ -418,11 +451,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) /* FIXME: Our current MAX_OPTION_LENGTH prevents us from receiving * an export name at the full NBD_MAX_STRING length. */ - assert (exportnamelen < sizeof conn->exportname); - memcpy (conn->exportname, &data[4], exportnamelen); - conn->exportname[exportnamelen] = '\0'; - debug ("newstyle negotiation: %s: client requested export '%s'", - optname, conn->exportname); + if (check_export_name (conn, option, &data[4], exportnamelen, + optlen - 6, true) == -1) + return -1; /* The spec is confusing, but it is required that we send back * NBD_INFO_EXPORT, even if the client did not request it! @@ -541,9 +572,13 @@ negotiate_handshake_newstyle_options (struct connection *conn) continue; } - /* Discard the export name. */ + /* Discard the export name, after validating it. */ memcpy (&exportnamelen, &data[0], 4); exportnamelen = be32toh (exportnamelen); + what = "validating export name"; + if (check_export_name (conn, option, &data[4], exportnamelen, + optlen - 8, false) == -1) + goto opt_meta_invalid_option_len; opt_index = 4 + exportnamelen; /* Read the number of queries. */ @@ -583,7 +618,8 @@ negotiate_handshake_newstyle_options (struct connection *conn) querylen = be32toh (querylen); opt_index += 4; what = "reading query string"; - if (opt_index + querylen > optlen) + if (check_string (option, &data[opt_index], querylen, + optlen - opt_index, "meta context query") == -1) goto opt_meta_invalid_option_len; debug ("newstyle negotiation: %s: %s %.*s", diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh index 7b0b43ad..86aefbaf 100755 --- a/tests/test-long-name.sh +++ b/tests/test-long-name.sh @@ -38,6 +38,7 @@ fail=0 # Test handling of NBD maximum string length of 4k. +requires qemu-io --version requires qemu-nbd --version name16=1234567812345678 @@ -71,6 +72,23 @@ case $out in fail=1 ;; esac +# Use largest possible export name, then oversize, with NBD_OPT_EXPORT_NAME. +nbdkit -U - --mask-handshake=0 null --run 'qemu-io -r -f raw -c quit \ + nbd+unix:///'$name4k'\?socket=$unixsocket' || fail=1 +# qemu 4.1 did not length check, letting it send an invalid NBD client +# request which nbdkit must filter out. Later qemu might refuse to +# send the request (like libnbd does), at which point this is no longer +# testing nbdkit proper, so we may remove it later: +nbdkit -U - --mask-handshake=0 null --run 'qemu-io -r -f raw -c quit \ + nbd+unix:///'a$name4k'\?socket=$unixsocket' && fail=1 + +# Repeat with NBD_OPT_GO. +nbdkit -U - null --run 'qemu-io -r -f raw -c quit \ + nbd+unix:///'$name1k$name1k'\?socket=$unixsocket' || fail=1 +# FIXME: Right now, we can't accept full 4k length - this should succeed +nbdkit -U - null --run 'qemu-io -r -f raw -c quit \ + nbd+unix:///'$almost4k'\?socket=$unixsocket' && fail=1 + # The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0. if ! qemu-nbd --help | grep -sq -- --list; then echo "$0: skipping because qemu-nbd does not support the --list option" -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT
Fixes the fact that clients could not request the maximum string length except with NBD_OPT_EXPORT_LEN. Updates the testsuite to match. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/protocol-handshake-newstyle.c | 12 +++++++----- tests/test-long-name.sh | 10 ++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 34958360..3b5d144e 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -48,7 +48,7 @@ #define MAX_NR_OPTIONS 32 /* Maximum length of any option data (bytes). */ -#define MAX_OPTION_LENGTH 4096 +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4) /* Receive newstyle options. */ static int @@ -255,7 +255,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) uint64_t version; uint32_t option; uint32_t optlen; - char data[MAX_OPTION_LENGTH+1]; + CLEANUP_FREE char *data = NULL; struct nbd_export_name_option_reply handshake_finish; const char *optname; uint64_t exportsize; @@ -281,6 +281,11 @@ negotiate_handshake_newstyle_options (struct connection *conn) nbdkit_error ("client option data too long (%" PRIu32 ")", optlen); return -1; } + data = malloc (optlen + 1); /* Allowing a trailing NUL helps some uses */ + if (data == NULL) { + nbdkit_error ("malloc: %m"); + return -1; + } option = be32toh (new_option.option); optname = name_of_nbd_opt (option); @@ -448,9 +453,6 @@ negotiate_handshake_newstyle_options (struct connection *conn) /* As with NBD_OPT_EXPORT_NAME we print the export name and * save it in the connection. */ - /* FIXME: Our current MAX_OPTION_LENGTH prevents us from receiving - * an export name at the full NBD_MAX_STRING length. - */ if (check_export_name (conn, option, &data[4], exportnamelen, optlen - 6, true) == -1) return -1; diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh index 86aefbaf..f9ebad6e 100755 --- a/tests/test-long-name.sh +++ b/tests/test-long-name.sh @@ -84,10 +84,10 @@ nbdkit -U - --mask-handshake=0 null --run 'qemu-io -r -f raw -c quit \ # Repeat with NBD_OPT_GO. nbdkit -U - null --run 'qemu-io -r -f raw -c quit \ - nbd+unix:///'$name1k$name1k'\?socket=$unixsocket' || fail=1 -# FIXME: Right now, we can't accept full 4k length - this should succeed + nbd+unix:///'$name4k'\?socket=$unixsocket' || fail=1 +# See above comment about whether this is testing nbdkit or qemu: nbdkit -U - null --run 'qemu-io -r -f raw -c quit \ - nbd+unix:///'$almost4k'\?socket=$unixsocket' && fail=1 + nbd+unix:///'a$name4k'\?socket=$unixsocket' && fail=1 # The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0. if ! qemu-nbd --help | grep -sq -- --list; then @@ -96,8 +96,6 @@ if ! qemu-nbd --help | grep -sq -- --list; then fi # Test response to NBD_OPT_LIST -nbdkit -U - -e $almost4k null --run 'qemu-nbd --list -k $unixsocket' || fail=1 -# FIXME: Right now, we can't accept full 4k length - this should succeed -nbdkit -U - -e $name4k null --run 'qemu-nbd --list -k $unixsocket' && fail=1 +nbdkit -U - -e $name4k null --run 'qemu-nbd --list -k $unixsocket' || fail=1 exit $fail -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 6/7] server: Fix OPT_GO on different export than SET_META_CONTEXT
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, but I also tested it via strategic gdb breakpoints in qemu-io. 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 | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/server/internal.h b/server/internal.h index e6a22f1a..97e417f9 100644 --- a/server/internal.h +++ b/server/internal.h @@ -200,6 +200,7 @@ struct connection { size_t nr_handles; char exportname[NBD_MAX_STRING + 1]; + 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 3b5d144e..2480d7a3 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -239,8 +239,12 @@ check_export_name (struct connection *conn, uint32_t option, char *buf, assert (exportnamelen < sizeof conn->exportname); if (save) { + if (exportnamelen != conn->exportnamelen || + memcmp (conn->exportname, buf, exportnamelen) != 0) + conn->meta_context_base_allocation = false; memcpy (conn->exportname, buf, exportnamelen); conn->exportname[exportnamelen] = '\0'; + conn->exportnamelen = exportnamelen; } debug ("newstyle negotiation: %s: client requested export '%.*s'", name_of_nbd_opt (option), (int) exportnamelen, buf); @@ -451,7 +455,9 @@ 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. 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 (check_export_name (conn, option, &data[4], exportnamelen, optlen - 6, true) == -1) @@ -574,12 +580,16 @@ negotiate_handshake_newstyle_options (struct connection *conn) continue; } - /* Discard the export name, after validating it. */ + /* 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 = "validating export name"; if (check_export_name (conn, option, &data[4], exportnamelen, - optlen - 8, false) == -1) + optlen - 8, + option == NBD_OPT_SET_META_CONTEXT) == -1) goto opt_meta_invalid_option_len; opt_index = 4 + exportnamelen; -- 2.21.0
Eric Blake
2019-Sep-28 04:48 UTC
[Libguestfs] [nbdkit PATCH v2 7/7] server: Better newstyle .open failure handling
If a plugin's .open or .get_size or .can_write fails, right now that is fatal to the connection. When nbdkit was first implemented, this made sense (there was no way to report errors to oldstyle or NBD_OPT_EXPORT_NAME). But now that newstyle is around, it's rather abrupt to hang up on the client, and better is to return an error to NBD_OPT_GO, and let the client choose what to do (most clients will probably hang up, whether gracefully with NBD_OPT_ABORT or abruptly, rather than try other NBD_OPT_*, but _we_ shouldn't be the ones forcing their hand). For an example of what this improves, if you run: $ nbdkit -fv sh - <<\EOF case $1 in get_size) echo oops >&2; exit 1;; *) exit 2;; esac EOF Pre-patch, qemu complains about the abrupt server death: $ qemu-nbd --list qemu-nbd: Failed to read option reply: Unexpected end-of-file before all bytes were read Post-patch, qemu gets the desired error message, and exits gracefully: $ qemu-nbd --list qemu-nbd: Requested export not available Note that this does not fix the pre-existing problem that we can end up calling .finalize even when .prepare was skipped or failed (that latent problem was first exposed for the rare client that calls NBD_OPT_INFO before NBD_OPT_GO, see commit a6b88b19); a later patch will have to add better bookkeeping for that, and for better handling of reopen requests from the retry filter. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/protocol-handshake-newstyle.c | 32 +++++++++++++++++++++------- server/protocol-handshake-oldstyle.c | 3 +++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 2480d7a3..878fe53f 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -198,7 +198,7 @@ conn_recv_full (struct connection *conn, void *buf, size_t len, /* Sub-function of negotiate_handshake_newstyle_options below. It * must be called on all non-error paths out of the options for-loop - * in that function. + * in that function, and must not cause any wire traffic. */ static int finish_newstyle_options (struct connection *conn, uint64_t *exportsize) @@ -322,7 +322,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) if (check_export_name (conn, option, data, optlen, optlen, true) == -1) return -1; - /* We have to finish the handshake by sending handshake_finish. */ + /* We have to finish the handshake by sending handshake_finish. + * On failure, we have to disconnect. + */ if (finish_newstyle_options (conn, &exportsize) == -1) return -1; @@ -460,16 +462,30 @@ negotiate_handshake_newstyle_options (struct connection *conn) * or else we drop the support for that context. */ if (check_export_name (conn, option, &data[4], exportnamelen, - optlen - 6, true) == -1) - return -1; + optlen - 6, true) == -1) { + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } /* The spec is confusing, but it is required that we send back * NBD_INFO_EXPORT, even if the client did not request it! * qemu client in particular does not request this, but will - * fail if we don't send it. + * fail if we don't send it. Note that if .open fails, but we + * succeed at .close, then we merely return an error to the + * client and let them try another NBD_OPT, rather than + * disconnecting. */ - if (finish_newstyle_options (conn, &exportsize) == -1) - return -1; + if (finish_newstyle_options (conn, &exportsize) == -1) { + if (backend->finalize (backend, conn) == -1) + return -1; + backend_close (backend, conn); + if (send_newstyle_option_reply (conn, option, + NBD_REP_ERR_UNKNOWN) == -1) + return -1; + continue; + } if (send_newstyle_option_reply_info_export (conn, option, NBD_REP_INFO, @@ -497,7 +513,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) } /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK - * or ERROR packet. + * or ERROR packet. If this was NBD_OPT_LIST, call .close. */ if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) return -1; diff --git a/server/protocol-handshake-oldstyle.c b/server/protocol-handshake-oldstyle.c index 4efc668b..45a1a486 100644 --- a/server/protocol-handshake-oldstyle.c +++ b/server/protocol-handshake-oldstyle.c @@ -52,6 +52,9 @@ protocol_handshake_oldstyle (struct connection *conn) assert (tls != 2); /* Already filtered in main */ + /* With oldstyle, our only option if .open or friends fail is to + * disconnect, as we cannot report the problem to the client. + */ if (protocol_common_open (conn, &exportsize, &eflags) == -1) return -1; -- 2.21.0
Richard W.M. Jones
2019-Sep-28 20:07 UTC
Re: [Libguestfs] [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT
On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote:> Fixes the fact that clients could not request the maximum string > length except with NBD_OPT_EXPORT_LEN. Updates the testsuite to > match. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/protocol-handshake-newstyle.c | 12 +++++++----- > tests/test-long-name.sh | 10 ++++------ > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c > index 34958360..3b5d144e 100644 > --- a/server/protocol-handshake-newstyle.c > +++ b/server/protocol-handshake-newstyle.c > @@ -48,7 +48,7 @@ > #define MAX_NR_OPTIONS 32 > > /* Maximum length of any option data (bytes). */ > -#define MAX_OPTION_LENGTH 4096 > +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4)I may have missed it - why was * 4 chosen? 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
Richard W.M. Jones
2019-Sep-28 20:08 UTC
Re: [Libguestfs] [nbdkit PATCH v2 6/7] server: Fix OPT_GO on different export than SET_META_CONTEXT
I'm good up to patch 6. I had a small comment about patch 5, but ACK I'll look at patch 7 later. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Richard W.M. Jones
2019-Sep-28 20:28 UTC
Re: [Libguestfs] [nbdkit PATCH v2 7/7] server: Better newstyle .open failure handling
On Fri, Sep 27, 2019 at 11:48:49PM -0500, Eric Blake wrote:> If a plugin's .open or .get_size or .can_write fails, right now that > is fatal to the connection. When nbdkit was first implemented, this > made sense (there was no way to report errors to oldstyle or > NBD_OPT_EXPORT_NAME). But now that newstyle is around, it's rather > abrupt to hang up on the client, and better is to return an error to > NBD_OPT_GO, and let the client choose what to do (most clients will > probably hang up, whether gracefully with NBD_OPT_ABORT or abruptly, > rather than try other NBD_OPT_*, but _we_ shouldn't be the ones > forcing their hand). > > For an example of what this improves, if you run: > > $ nbdkit -fv sh - <<\EOF > case $1 in get_size) echo oops >&2; exit 1;; *) exit 2;; esac > EOF > > Pre-patch, qemu complains about the abrupt server death: > $ qemu-nbd --list > qemu-nbd: Failed to read option reply: Unexpected end-of-file before all bytes were read > > Post-patch, qemu gets the desired error message, and exits gracefully: > $ qemu-nbd --list > qemu-nbd: Requested export not available > > Note that this does not fix the pre-existing problem that we can end > up calling .finalize even when .prepare was skipped or failed (that > latent problem was first exposed for the rare client that calls > NBD_OPT_INFO before NBD_OPT_GO, see commit a6b88b19); a later patch > will have to add better bookkeeping for that, and for better handling > of reopen requests from the retry filter. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/protocol-handshake-newstyle.c | 32 +++++++++++++++++++++------- > server/protocol-handshake-oldstyle.c | 3 +++ > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c > index 2480d7a3..878fe53f 100644 > --- a/server/protocol-handshake-newstyle.c > +++ b/server/protocol-handshake-newstyle.c > @@ -198,7 +198,7 @@ conn_recv_full (struct connection *conn, void *buf, size_t len, > > /* Sub-function of negotiate_handshake_newstyle_options below. It > * must be called on all non-error paths out of the options for-loop > - * in that function. > + * in that function, and must not cause any wire traffic. > */ > static int > finish_newstyle_options (struct connection *conn, uint64_t *exportsize) > @@ -322,7 +322,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) > if (check_export_name (conn, option, data, optlen, optlen, true) == -1) > return -1; > > - /* We have to finish the handshake by sending handshake_finish. */ > + /* We have to finish the handshake by sending handshake_finish. > + * On failure, we have to disconnect. > + */ > if (finish_newstyle_options (conn, &exportsize) == -1) > return -1; > > @@ -460,16 +462,30 @@ negotiate_handshake_newstyle_options (struct connection *conn) > * or else we drop the support for that context. > */ > if (check_export_name (conn, option, &data[4], exportnamelen, > - optlen - 6, true) == -1) > - return -1; > + optlen - 6, true) == -1) { > + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) > + == -1) > + return -1; > + continue; > + } > > /* The spec is confusing, but it is required that we send back > * NBD_INFO_EXPORT, even if the client did not request it! > * qemu client in particular does not request this, but will > - * fail if we don't send it. > + * fail if we don't send it. Note that if .open fails, but we > + * succeed at .close, then we merely return an error to the > + * client and let them try another NBD_OPT, rather than > + * disconnecting. > */ > - if (finish_newstyle_options (conn, &exportsize) == -1) > - return -1; > + if (finish_newstyle_options (conn, &exportsize) == -1) { > + if (backend->finalize (backend, conn) == -1) > + return -1; > + backend_close (backend, conn); > + if (send_newstyle_option_reply (conn, option, > + NBD_REP_ERR_UNKNOWN) == -1) > + return -1; > + continue; > + } > > if (send_newstyle_option_reply_info_export (conn, option, > NBD_REP_INFO, > @@ -497,7 +513,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) > } > > /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK > - * or ERROR packet. > + * or ERROR packet. If this was NBD_OPT_LIST, call .close. > */ > if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) > return -1; > diff --git a/server/protocol-handshake-oldstyle.c b/server/protocol-handshake-oldstyle.c > index 4efc668b..45a1a486 100644 > --- a/server/protocol-handshake-oldstyle.c > +++ b/server/protocol-handshake-oldstyle.c > @@ -52,6 +52,9 @@ protocol_handshake_oldstyle (struct connection *conn) > > assert (tls != 2); /* Already filtered in main */ > > + /* With oldstyle, our only option if .open or friends fail is to > + * disconnect, as we cannot report the problem to the client. > + */ > if (protocol_common_open (conn, &exportsize, &eflags) == -1) > return -1;Yes this seems fine. Probably best to check that the series doesn't break ‘make check-valgrind’ before pushing however. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Richard W.M. Jones
2019-Sep-28 21:38 UTC
Re: [Libguestfs] [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT
On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote:> Fixes the fact that clients could not request the maximum string > length except with NBD_OPT_EXPORT_LEN. Updates the testsuite to > match. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/protocol-handshake-newstyle.c | 12 +++++++----- > tests/test-long-name.sh | 10 ++++------ > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c > index 34958360..3b5d144e 100644 > --- a/server/protocol-handshake-newstyle.c > +++ b/server/protocol-handshake-newstyle.c > @@ -48,7 +48,7 @@ > #define MAX_NR_OPTIONS 32 > > /* Maximum length of any option data (bytes). */ > -#define MAX_OPTION_LENGTH 4096 > +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4) > > /* Receive newstyle options. */ > static int > @@ -255,7 +255,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) > uint64_t version; > uint32_t option; > uint32_t optlen; > - char data[MAX_OPTION_LENGTH+1]; > + CLEANUP_FREE char *data = NULL;Even though you have the CLEANUP here ...> struct nbd_export_name_option_reply handshake_finish; > const char *optname; > uint64_t exportsize; > @@ -281,6 +281,11 @@ negotiate_handshake_newstyle_options (struct connection *conn) > nbdkit_error ("client option data too long (%" PRIu32 ")", optlen); > return -1; > } > + data = malloc (optlen + 1); /* Allowing a trailing NUL helps some uses */ > + if (data == NULL) { > + nbdkit_error ("malloc: %m"); > + return -1; > + }... when I run this patch series under valgrind I get mainly errors originating at this malloc: ==1251605== 58 bytes in 4 blocks are definitely lost in loss record 4 of 10 ==1251605== at 0x896180B: malloc (vg_replace_malloc.c:309) ==1251605== by 0x11909F: protocol_handshake_newstyle (protocol-handshake-news tyle.c:288) ==1251605== by 0x118804: protocol_handshake (protocol-handshake.c:55) ==1251605== by 0x112080: handle_single_connection (connections.c:165) ==1251605== by 0x11B84D: start_thread (sockets.c:276) ==1251605== by 0x8BB74E1: start_thread (pthread_create.c:479) ==1251605== by 0x8CD3642: clone (clone.S:95) I didn't look at it closely but there does appear to be a memory leak in this patch. 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