On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
> Always allocate space for the reply returned by the server and
> hoist the trace earlier, as it is more interesting to trace the
> server's reply (even if it is unexpected) than parroting our
> request only on success. After all, skipping the allocation
> for a wrong size was merely a micro-optimization that only
> benefitted a broken server, rather than the common case of a
> compliant server that meets our expectations.
>
> Then turn the reply handling into a loop (even though we still
> never iterate more than once), to make this code easier to use
> when later patches do support multiple server replies. This
> changes the error message for a server with two replies (a
> corner case we are unlikely to hit in practice) from:
>
> Unexpected reply type 4 (meta context), expected 0 (ack)
>
> to:
>
> Server replied with more than one context
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> ---
> nbd/client.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index bcccd5f555e..b6a85fc3ef8 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return ret;
> }
>
> - if (reply.type == NBD_REP_META_CONTEXT) {
> + while (reply.type == NBD_REP_META_CONTEXT) {
I'm not sure I understand why this change is safe.
As far as I can see reply.type is only updated in the loop by
nbd_receive_option_reply, and that reads from the server, and so the
server might keep sending NBD_REP_META_CONTEXT packets (instead of the
expected NBD_REP_ACK), so it could now loop forever against a
malicious server? (This is not taking into account any later patches)
Rich.
> char *name;
>
> - if (reply.length != sizeof(info->context_id) + context_len) {
> + if (reply.length <= sizeof(info->context_id) ||
> + reply.length > NBD_MAX_BUFFER_SIZE) {
> error_setg(errp, "Failed to negotiate meta context '%s', server "
> "answered with unexpected length %" PRIu32, context,
> reply.length);
> @@ -708,6 +709,15 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
> name[reply.length] = '\0';
> + trace_nbd_opt_meta_reply(name, info->context_id);
> +
> + if (received) {
> + error_setg(errp, "Server replied with more than one context");
> + g_free(name);
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
> +
> if (strcmp(context, name)) {
> error_setg(errp, "Failed to negotiate meta context '%s', server "
> "answered with different context '%s'", context,
> @@ -717,8 +727,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
> g_free(name);
> -
> - trace_nbd_opt_meta_reply(context, info->context_id);
> received = true;
>
> /* receive NBD_REP_ACK */
> --
> 2.17.2
--
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
