[2/3] vnc: added initial websocket protocol support

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:



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

Tim Hardeck <thardeck@suse.de> writes:

> 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:

Better, but I still think it's better to advance the buffer based on the
parsed header sizes that to assume there's no additional data.

Regards,

Anthony Liguori

>
> 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/

On 01/08/2013 01:38 AM, Anthony Liguori wrote:
> Better, but I still think it's better to advance the buffer based on the
> parsed header sizes that to assume there's no additional data.

I have used buffer_advance in the patch set v6. Does anything else need
to be changed?

Regards
Tim