nbdkit_error has traditionally been a "fancy wrapper around fprintf" (kind of, don't take that literally). It is encouraged that plugins and filters do something like: if (error) { nbdkit_error ("oops, a bad thing happened"); return -1; } but we don't enforce this. Plugins might call nbdkit_error more than once or not at all. The point where we get to sending an error back over the wire to the NBD client is long after the plugin returned above, and after nbdkit_error was called. Therefore in order to send errors back to the NBD client, we must keep the last error message around. This change simply modifies nbdkit_error to make a best-effort attempt to save the last error message in the connection struct, and provides a way to access that last error. --- server/internal.h | 4 ++++ server/connections.c | 49 ++++++++++++++++++++++++++++++++++++++++++++ server/log.c | 25 ++++++++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/server/internal.h b/server/internal.h index 1a783e3fc0..fa74ea22af 100644 --- a/server/internal.h +++ b/server/internal.h @@ -274,6 +274,7 @@ struct connection { int status_pipe[2]; /* track status changes via poll when nworkers > 1 */ void *crypto_session; int nworkers; + char *last_error; struct context *top_context; /* The context tied to 'top'. */ char **default_exportname; /* One per plugin and filter. */ @@ -300,6 +301,9 @@ struct connection { extern void handle_single_connection (int sockin, int sockout); extern conn_status connection_get_status (void); extern bool connection_set_status (conn_status value); +extern void connection_set_last_error (char *msg); +extern void connection_clear_last_error (void); +extern char *connection_get_last_error (void); /* protocol-handshake.c */ extern int protocol_handshake (void); diff --git a/server/connections.c b/server/connections.c index 13b67f3f96..fe534da934 100644 --- a/server/connections.c +++ b/server/connections.c @@ -111,6 +111,54 @@ connection_set_status (conn_status value) return ret; } +/* Set the last_error field in the connection. The ownership of the + * 'msg' string is passed to the connection and will be freed here. + */ +void +connection_set_last_error (char *msg) +{ + GET_CONN; + + if (conn->nworkers && + pthread_mutex_lock (&conn->status_lock)) + abort (); + free (conn->last_error); + conn->last_error = msg; + if (conn->nworkers && + pthread_mutex_unlock (&conn->status_lock)) + abort (); +} + +/* Clear the last_error field in the connection handle. */ +void +connection_clear_last_error (void) +{ + connection_set_last_error (NULL); +} + +/* Get a copy of the last_error field in the connection handle. If + * successful, this returns a non-NULL string that the caller must + * free. Returning NULL is not necessarily an error. The last_error + * is informational and may not be available. + */ +char * +connection_get_last_error (void) +{ + GET_CONN; + char *r = NULL; + + if (conn->nworkers && + pthread_mutex_lock (&conn->status_lock)) + abort (); + if (conn->last_error) + r = strdup (conn->last_error); + if (conn->nworkers && + pthread_mutex_unlock (&conn->status_lock)) + abort (); + + return r; +} + struct worker_data { struct connection *conn; char *name; @@ -391,6 +439,7 @@ free_connection (struct connection *conn) for_each_backend (b) free (conn->default_exportname[b->i]); free (conn->default_exportname); + free (conn->last_error); free (conn); threadlocal_set_conn (NULL); diff --git a/server/log.c b/server/log.c index 9c1f667a9b..37aa2c3494 100644 --- a/server/log.c +++ b/server/log.c @@ -40,6 +40,29 @@ #include "internal.h" +/* Copy the error message to the connection handle, if there is one. + * This is sent to callers which are using structured replies, but is + * for extra information only so don't fail if we are unable to copy + * it. + */ +static void +copy_error_to_connection (int orig_errno, const char *fs, va_list args) +{ + struct connection *conn = threadlocal_get_conn (); + va_list args_copy; + char *msg; + int r; + + if (!conn) return; + + va_copy (args_copy, args); + errno = orig_errno; /* must restore in case fs contains %m */ + r = vasprintf (&msg, fs, args_copy); + va_end (args_copy); + if (r != -1 && msg) + connection_set_last_error (msg); /* ownership passed to connection */ +} + /* Call the right log_*_verror function depending on log_sink. * Note: preserves the previous value of errno. */ @@ -48,6 +71,8 @@ log_verror (const char *fs, va_list args) { int orig_errno = errno; + copy_error_to_connection (orig_errno, fs, args); + switch (log_to) { case LOG_TO_DEFAULT: if (forked_into_background) -- 2.44.0

This sends the last error saved in the connection handle back to the NBD client. This is informational and best effort. Errors are isolated by connection, so they shouldn't leak between connections (which would be a security issue). However there are unavoidable cases where we leak errors across callbacks: * When there are multiple callbacks running in parallel on a single connection (the parallel thread model), nbdkit_error might be called in another callback before we get to report the first error. In theory this could be solved by storing errors both by connection & in thread-local storage, but that's complicated to implement. * If the plugin does not consistently call nbdkit_error along error paths then an error from an earlier callback might be reported in an unrelated callback that does not call nbdkit_error. We could get around this by resetting the error at the beginning of a callback, although that is itself problematic in the parallel case because of the previous point. --- server/protocol-handshake-newstyle.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 6b3bc76f0f..15e90e0325 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -57,16 +57,31 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply) { GET_CONN; struct nbd_fixed_new_option_reply fixed_new_option_reply; + CLEANUP_FREE char *last_error = NULL; + uint32_t replylen = 0; + + if (NBD_REP_IS_ERR (reply)) { + size_t len; + + last_error = connection_get_last_error (); + if (last_error) { + len = strlen (last_error); + if (len <= NBD_MAX_STRING) + replylen = len; + } + } fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); fixed_new_option_reply.option = htobe32 (option); fixed_new_option_reply.reply = htobe32 (reply); - fixed_new_option_reply.replylen = htobe32 (0); + fixed_new_option_reply.replylen = htobe32 (replylen); debug ("replying to %s with %s", name_of_nbd_opt (option), name_of_nbd_rep (reply)); if (conn->send (&fixed_new_option_reply, - sizeof fixed_new_option_reply, 0) == -1) { + sizeof fixed_new_option_reply, + replylen > 0 ? SEND_MORE : 0) == -1) { + err: /* The protocol document says that the client is allowed to simply * drop the connection after sending NBD_OPT_ABORT, or may read * the reply. @@ -77,6 +92,8 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply) nbdkit_error ("write: %s: %m", name_of_nbd_opt (option)); return -1; } + if (replylen > 0 && conn->send (last_error, replylen, 0) == -1) + goto err; return 0; } -- 2.44.0