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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote: 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

On Fri, Sep 27, 2019 at 11:48:49PM -0500, Eric Blake wrote: 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

On 9/28/19 3:07 PM, Richard W.M. Jones wrote: NBD_OPT_SET_META_CONTEXT allows two strings plus a few glue bytes, so more than 8k of data from a compliant client. 16k is the next power of 2. We can bump it larger if we want, especially since 16k pales in comparison to our 32M limit on NBD_CMD_WRITE, but for now, there is nothing in the NBD protocol larger than NBD_OPT_SET_META_CONTEXT. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

On Mon, Sep 30, 2019 at 11:22:03AM -0500, Eric Blake wrote: Ah yes of course, I didn't spot that there was a loop :-) 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