Hi Anthony, thanks for your feedback.
On Mon, 2013-01-07 at 13:52 -0600, Anthony Liguori wrote:
> Tim Hardeck <thard...@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/
signature.asc
Description: This is a digitally signed message part
