Re: [Qemu-devel] [PATCH 1/1] io/channel-websock: handle continuous reads without any data

[Daniel P. Berrange]

On Thu, Dec 28, 2017 at 12:36:18PM +0300, Denis V. Lunev wrote:
> From: Edgar Kaziakhmedov <address@hidden>
> 
> According to the current implementation of websocket protocol in QEMU,
> qio_channel_websock_handshake_io tries to read handshake from the
> channel to start communication over tcp socket. But this subroutine
> doesn't cover scenario when socket was closed while handshaking. Therefore,
> if G_IO_IN is caught and qio_channel_websock_handshake_read returns
> zero, error has to be set and connection has to be done.
> 
> Such behaviour causes 100% CPU load in main QEMU loop, because main loop
> poll continues to receive and handle G_IO_IN events from websocket.
> 
> Step to reproduce 100% CPU load:
> - start qemu with the simplest configuration
>     $ qemu -vnc [::1]:1,websocket=7500
> 2) open any vnc listener (which doesn't follow websocket protocol)
>     $ vncviewer :7500
> 3) kill listener
> 4) qemu main thread eats 100% CPU usage
> 
> Signed-off-by: Edgar Kaziakhmedov <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Daniel P. Berrange <address@hidden>
> ---
>  io/channel-websock.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 87ebdeb..71f046a 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -581,9 +581,11 @@ static gboolean 
> qio_channel_websock_handshake_io(QIOChannel *ioc,
>          return FALSE;
>      }
>      if (ret == 0) {
> -        trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN);
> -        /* need more data still */
> -        return TRUE;
> +        /* If G_IN_IO happened and there is no data to read, close 
> connection */
> +        error_setg(&err, "connection was closed");
> +        qio_task_set_error(task, err);
> +        qio_task_complete(task);
> +        return FALSE;

This is bogus - 'ret == 0' from  qio_channel_websock_handshake_read()
does *not* imply end of file.

It says we have read some data, but not yet seen the end of the HTTP
headers.

If you want to detect EOF, you need to check the qio_channel_read()
function return value directly in that method.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|