Re: [Qemu-devel] [PATCH 2/3] vnc: added initial websocket protocol support

[Tim Hardeck <thardeck@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]

Hi Anthony,

thanks for your feedback.

On Mon, 2013-01-07 at 13:52 -0600, Anthony Liguori wrote:
> Tim Hardeck <thardeck@suse.de> 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:

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)


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


-- 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix
Imendörffer, HRB 16746 (AG Nürnberg)
Maxfeldstr. 5, 90409 Nürnberg, Germany
T: +49 (0) 911 74053-0  F: +49 (0) 911 74053-483
http://www.suse.de/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]