From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53150) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gYBjA-00080D-Tv for qemu-devel@nongnu.org; Sat, 15 Dec 2018 10:19:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gYBj9-0003AA-Pb for qemu-devel@nongnu.org; Sat, 15 Dec 2018 10:19:40 -0500 Date: Sat, 15 Dec 2018 15:19:29 +0000 From: "Richard W.M. Jones" Message-ID: <20181215151929.GY27120@redhat.com> References: <20181215135324.152629-1-eblake@redhat.com> <20181215135324.152629-13-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181215135324.152629-13-eblake@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, nsoffer@redhat.com, jsnow@redhat.com, vsementsov@virtuozzo.com, qemu-block@nongnu.org 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 > > --- > 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