search for: negotiate_handshake_newstyle_opt

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

...style.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; >...

...xportnamelen; 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; @@...

...} } - return send_newstyle_option_reply (option, NBD_REP_ACK); + r = send_newstyle_option_reply (option, NBD_REP_ACK); + /* Allow additional per-export NBD_OPT_INFO and friends. */ + if (r == 0) + *nr_options += MAX_NR_OPTIONS * list_len; + return r; } static int @@ -326,6 +335,7 @@ negotiate_handshake_newstyle_options (void) GET_CONN; struct nbd_new_option new_option; size_t nr_options; + bool list_seen = false; uint64_t version; uint32_t option; uint32_t optlen; @@ -334,7 +344,7 @@ negotiate_handshake_newstyle_options (void) uint64_t exportsize; struct backend *b; - for (nr_option...

.../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_new...

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

...ake-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_ha...

...rtnamelen, uint32_t maxlen, bool save) { + struct connection *conn = GET_CONN; + if (check_string (option, buf, exportnamelen, maxlen, "export name") == -1) return -1; @@ -254,8 +254,9 @@ check_export_name (struct connection *conn, uint32_t option, char *buf, } static int -negotiate_handshake_newstyle_options (struct connection *conn) +negotiate_handshake_newstyle_options (void) { + struct connection *conn = GET_CONN; struct nbd_new_option new_option; size_t nr_options; uint64_t version; @@ -268,7 +269,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) for (nr_options...

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

...context) == -1 || + conn->send (conn, name, namelen) == -1) { + nbdkit_error ("write: %m"); + return -1; + } + + return 0; +} + /* Sub-function during negotiate_handshake_newstyle, to uniformly handle * a client hanging up on a message boundary. */ @@ -452,6 +480,8 @@ negotiate_handshake_newstyle_options (struct connection *conn) case NBD_OPT_LIST_META_CONTEXT: case NBD_OPT_SET_META_CONTEXT: { + int r; + bool can_extents; uint32_t opt_index; uint32_t exportnamelen; uint32_t nr_queries; @@ -469,6 +499,16 @@ negotiate_handshake_newstyle_op...

..._new_option_reply; + struct nbd_fixed_new_option_reply_meta_context context; const size_t namelen = strlen (name); debug ("newstyle negotiation: %s: replying with %s id %d", @@ -213,13 +213,13 @@ finish_newstyle_options (struct connection *conn, uint64_t *exportsize) static int negotiate_handshake_newstyle_options (struct connection *conn) { - struct new_option new_option; + struct nbd_new_option new_option; size_t nr_options; uint64_t version; uint32_t option; uint32_t optlen; char data[MAX_OPTION_LENGTH+1]; - struct new_handshake_finish handshake_finish; + struct nbd_new_handshake_f...

...500, 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> >> --- >> @@ -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 ... Scope is too wide. I need to sink t...

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

...7 @@ finish_newstyle_options (struct connection *conn) "(%" PRIi64 ")", r); return -1; } - conn->exportsize = (uint64_t) r; + *exportsize = r; if (protocol_compute_eflags (conn, &conn->eflags) < 0) return -1; @@ -233,6 +233,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) char data[MAX_OPTION_LENGTH+1]; struct new_handshake_finish handshake_finish; const char *optname; + uint64_t exportsize; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { if (conn_recv_full (conn, &new_option, sizeof new_optio...

...context) == -1 || + conn->send (conn, name, namelen) == -1) { + nbdkit_error ("write: %m"); + return -1; + } + + return 0; +} + /* Sub-function during negotiate_handshake_newstyle, to uniformly handle * a client hanging up on a message boundary. */ @@ -452,6 +480,8 @@ negotiate_handshake_newstyle_options (struct connection *conn) case NBD_OPT_LIST_META_CONTEXT: case NBD_OPT_SET_META_CONTEXT: { + int r; + bool can_extents; uint32_t opt_index; uint32_t exportnamelen; uint32_t nr_queries; @@ -469,6 +499,16 @@ negotiate_handshake_newstyle_op...

This is the sort of thing I had in mind for option (1) here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html It does reveal that the way we currently list exports is naive to say the least ... Rich.

...*exportname; uint32_t cflags; uint16_t eflags; bool using_tls; diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 9ddc319..e1301a0 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -274,11 +274,17 @@ 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; - /* Apart from printing it, ignore the export name. */ + /* Print the export name and save it in the conn...

...s if it connected to the export named "". After all, qemu 3.1 was where we changed things to allow a client to request an explicit export name of "" yet still connect to an oldstyle server in that case. > +++ b/server/protocol-handshake-newstyle.c > @@ -274,11 +274,17 @@ 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; > - /* Apart from printing it, ignore the export name. */ > + /* Print the export nam...

....can_fua = plugin_can_fua, .can_multi_conn = plugin_can_multi_conn, diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index e0136de1..518de703 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -233,6 +233,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) char data[MAX_OPTION_LENGTH+1]; struct new_handshake_finish handshake_finish; const char *optname; + int r; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { if (conn_recv_full (conn, &new_option, sizeof new_option, @@ -481,6 +...