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

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 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

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