From patchwork Mon Jan 7 23:57:01 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Hardeck X-Patchwork-Id: 210273 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id F2EA92C0098 for ; Tue, 8 Jan 2013 10:58:06 +1100 (EST) Received: from localhost ([::1]:48748 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsMZx-0004Rz-6T for incoming@patchwork.ozlabs.org; Mon, 07 Jan 2013 18:58:05 -0500 Received: from eggs.gnu.org ([208.118.235.92]:37971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsMZo-0004OK-VK for qemu-devel@nongnu.org; Mon, 07 Jan 2013 18:57:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsMZn-00026w-Bk for qemu-devel@nongnu.org; Mon, 07 Jan 2013 18:57:56 -0500 Received: from cantor2.suse.de ([195.135.220.15]:58255 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsMZn-00020x-2u for qemu-devel@nongnu.org; Mon, 07 Jan 2013 18:57:55 -0500 Received: from relay2.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 0D209A4E0C; Tue, 8 Jan 2013 00:57:13 +0100 (CET) Message-ID: <1357603021.1939.64.camel@Thinktank.site> From: Tim Hardeck To: Anthony Liguori Date: Tue, 08 Jan 2013 00:57:01 +0100 In-Reply-To: <87sj6cu5y0.fsf@codemonkey.ws> References: <1357133386-7643-1-git-send-email-thardeck@suse.de> <1357133386-7643-3-git-send-email-thardeck@suse.de> <87sj6cu5y0.fsf@codemonkey.ws> Organization: SUSE LINUX Products GmbH X-Mailer: Evolution 3.4.4 Mime-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x X-Received-From: 195.135.220.15 Cc: stefanha@gmail.com, github@martintribe.org, qemu-devel@nongnu.org, alevy@redhat.com, kraxel@redhat.com, corentin.chary@gmail.com Subject: Re: [Qemu-devel] [PATCH 2/3] vnc: added initial websocket protocol support X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi Anthony, thanks for your feedback. On Mon, 2013-01-07 at 13:52 -0600, Anthony Liguori wrote: > Tim Hardeck writes: > > +void vncws_handshake_read(void *opaque) > > +{ > > + VncState *vs = opaque; > > + long ret; > > + buffer_reserve(&vs->ws_input, 4096); > > + ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096); > > + > > + if (!ret) { > > + if (vs->csock == -1) { > > + vnc_disconnect_finish(vs); > > + } > > + return; > > + } > > + vs->ws_input.offset += ret; > > + > > + if (vs->ws_input.offset > 0) { > > + qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs); > > + vncws_process_handshake(vs, vs->ws_input.buffer, vs->ws_input.offset); > > + buffer_reset(&vs->ws_input); > > This is making a bad assumption. You're assuming that > vnc_client_read_buf() is always going to return at least the amount of > data you need to do the handshake and never any more data. Following the Websocket protocol specification there shouldn't be more data until a handshake response from the server. > You need something more sophisticated that keeps reading data until > you've gotten the complete set of headers and the trailing double EOL. How about that: We also could calculate the handshake size and use buffer_advance instead but since there shouldn't be any additional data received from the client until a server response anyway I would prefer it this way. > > > +long vnc_client_read_ws(VncState *vs) > > +{ > > + int ret, err; > > + uint8_t *payload; > > + size_t payload_size, frame_size; > > + VNC_DEBUG("Read websocket %p size %zd offset %zd\n", vs->ws_input.buffer, > > + vs->ws_input.capacity, vs->ws_input.offset); > > + buffer_reserve(&vs->ws_input, 4096); > > + ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096); > > + if (!ret) { > > + return 0; > > + } > > + vs->ws_input.offset += ret; > > + > > + /* make sure that nothing is left in the ws_input buffer */ > > + do { > > + err = vncws_decode_frame(&vs->ws_input, &payload, > > + &payload_size, &frame_size); > > + if (err <= 0) { > > + return err; > > + } > > + > > + buffer_reserve(&vs->input, payload_size); > > + buffer_append(&vs->input, payload, payload_size); > > + > > + buffer_advance(&vs->ws_input, frame_size); > > + } while (vs->ws_input.offset > 0); > > This likewise seems wrong. What happens if you get a read of a partial > frame? This code will fail, no? In case of a partial frame err would be 0 and QEMU would wait for more data until processing. Regards Tim diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c index 6b98d6b..298bc20 100644 --- a/ui/vnc-ws.c +++ b/ui/vnc-ws.c @@ -35,7 +35,8 @@ void vncws_handshake_read(void *opaque) } vs->ws_input.offset += ret; - if (vs->ws_input.offset > 0) { + if (g_strstr_len((char *)vs->ws_input.buffer, vs->ws_input.offset, + WS_HANDSHAKE_END)) { qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs); vncws_process_handshake(vs, vs->ws_input.buffer, vs->ws_input.offset); buffer_reset(&vs->ws_input); diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h index 7402e77..c8dfe34 100644 --- a/ui/vnc-ws.h +++ b/ui/vnc-ws.h @@ -38,6 +38,7 @@ Sec-WebSocket-Accept: %s\r\n\ Sec-WebSocket-Protocol: binary\r\n\ \r\n" #define WS_HANDSHAKE_DELIM "\r\n" +#define WS_HANDSHAKE_END "\r\n\r\n" #define WS_SUPPORTED_VERSION "13" #define WS_HEAD_MIN_LEN sizeof(uint16_t)