5:05 p.m.
Right now, our generator ensures that we are in the Negotiating state
before a client can call nbd_add_meta_context() while there is an
active server connection. That is, even if the user piles up enough
meta contexts that a slow-response server then causes nbd_aio_opt_go()
and friends to return early on blocked writes, with the state machine
now tracking pointers into the middle of the global list, those
pointers are not at risk of becoming a use-after-free because the user
cannot successfully call nbd_add_meta_context() to alter the global
list until they first resume the state machine (using nbd_aio_poll or
similar) to finish flushing the client's write to the server and
eventually get back to the Negotiating state.
However, it took me a while to perform that audit to ensure that the
user can't mess with the global list while the state machine is
peering into the middle of it (that is, state machine interlocks act
at a distance). Easier is to have the state machine act on a local
copy, which cannot be directly altered by user action regardless of
the current state; and therefore even if we later change our mind
about which states can call nbd_add_meta_context() (the state
restrictions in API.ml), that change won't accidentally cause our
state machine to start seeing corrupted list contents on the uncommon
case of blocked writes (in states-newstyle-opt-meta-context.c).
In this patch, I added a helper function to copy from either a
StringList (no clients of that parameter in this patch, but coming
soon) or from the implicit list. Also, at the time of this patch, the
copying is done once in the state machine, but later patches will
hoist it earlier into lib/opt.c as part of adding more APIs for
fine-grained control over nbd_set_opt_mode management of context
lists, starting with nbd_opt_list_meta_context_queries() which will
take an explicit list of meta contexts to query.
---
lib/internal.h | 2 ++
generator/states-newstyle-opt-meta-context.c | 16 +++++----
lib/handle.c | 1 +
lib/utils.c | 36 ++++++++++++++++++++
4 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 04bd8134..308e65ef 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -285,6 +285,7 @@ struct nbd_handle {
int connect_errno;
/* When sending metadata contexts, this is used. */
+ string_vector querylist;
size_t querynum;
/* When receiving block status, this is used. */
@@ -478,6 +479,7 @@ extern int nbd_internal_aio_get_direction (enum state state);
extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
extern int nbd_internal_copy_string_list (string_vector *v, char **in);
extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv);
+extern int nbd_internal_set_querylist (struct nbd_handle *h, char **in);
extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len);
extern void nbd_internal_fork_safe_perror (const char *s);
extern char *nbd_internal_printable_buffer (const void *buf, size_t count);
diff --git a/generator/states-newstyle-opt-meta-context.c
b/generator/states-newstyle-opt-meta-context.c
index bfc51eb4..31e84f35 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -65,10 +65,14 @@ NEWSTYLE.OPT_META_CONTEXT.START:
assert (!h->meta_valid);
+ if (nbd_internal_set_querylist (h, NULL) == -1) {
+ SET_NEXT_STATE (%.DEAD);
+ return 0;
+ }
/* Calculate the length of the option request data. */
len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */;
- for (i = 0; i < h->request_meta_contexts.len; ++i)
- len += 4 /* length of query */ + strlen (h->request_meta_contexts.ptr[i]);
+ for (i = 0; i < h->querylist.len; ++i)
+ len += 4 /* length of query */ + strlen (h->querylist.ptr[i]);
h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
h->sbuf.option.option = htobe32 (opt);
@@ -107,7 +111,7 @@ NEWSTYLE.OPT_META_CONTEXT.SEND_EXPORTNAME:
switch (send_from_wbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 0:
- h->sbuf.nrqueries = htobe32 (h->request_meta_contexts.len);
+ h->sbuf.nrqueries = htobe32 (h->querylist.len);
h->wbuf = &h->sbuf;
h->wlen = sizeof h->sbuf.nrqueries;
h->wflags = MSG_MORE;
@@ -125,12 +129,12 @@ NEWSTYLE.OPT_META_CONTEXT.SEND_NRQUERIES:
return 0;
NEWSTYLE.OPT_META_CONTEXT.PREPARE_NEXT_QUERY:
- if (h->querynum >= h->request_meta_contexts.len) {
+ if (h->querynum >= h->querylist.len) {
/* end of list of requested meta contexts */
SET_NEXT_STATE (%PREPARE_FOR_REPLY);
return 0;
}
- const char *query = h->request_meta_contexts.ptr[h->querynum];
+ const char *query = h->querylist.ptr[h->querynum];
h->sbuf.len = htobe32 (strlen (query));
h->wbuf = &h->sbuf.len;
@@ -140,7 +144,7 @@ NEWSTYLE.OPT_META_CONTEXT.PREPARE_NEXT_QUERY:
return 0;
NEWSTYLE.OPT_META_CONTEXT.SEND_QUERYLEN:
- const char *query = h->request_meta_contexts.ptr[h->querynum];
+ const char *query = h->querylist.ptr[h->querynum];
switch (send_from_wbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
diff --git a/lib/handle.c b/lib/handle.c
index d4426260..6f262ba3 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -133,6 +133,7 @@ nbd_close (struct nbd_handle *h)
/* Free user callbacks first. */
nbd_unlocked_clear_debug_callback (h);
+ string_vector_reset (&h->querylist);
free (h->bs_entries);
nbd_internal_reset_size_and_flags (h);
for (i = 0; i < h->meta_contexts.len; ++i)
diff --git a/lib/utils.c b/lib/utils.c
index da3cee72..7b514421 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -93,6 +93,42 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
return 0;
}
+/* Copy queries (defaulting to h->request_meta_contexts) into h->querylist.
+ * Set an error on failure.
+ */
+int
+nbd_internal_set_querylist (struct nbd_handle *h, char **queries)
+{
+ size_t i;
+
+ if (queries) {
+ if (nbd_internal_copy_string_list (&h->querylist, queries) == -1) {
+ set_error (errno, "realloc");
+ return -1;
+ }
+ /* Drop trailing NULL */
+ assert (h->querylist.len > 0);
+ string_vector_remove (&h->querylist, h->querylist.len - 1);
+ }
+ else {
+ string_vector_reset (&h->querylist);
+ for (i = 0; i < h->request_meta_contexts.len; ++i) {
+ char *copy = strdup (h->request_meta_contexts.ptr[i]);
+ if (copy == NULL) {
+ set_error (errno, "strdup");
+ return -1;
+ }
+ if (string_vector_append (&h->querylist, copy) == -1) {
+ set_error (errno, "realloc");
+ free (copy);
+ return -1;
+ }
+ }
+ }
+
+ return 0;
+}
+
/* Like sprintf ("%ld", v), but safe to use between fork and exec. Do
* not use this function in any other context.
*
--
2.37.3