* [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data
@ 2018-01-10 15:13 Edgar Kaziakhmedov
2018-01-10 15:22 ` Daniel P. Berrange
0 siblings, 1 reply; 4+ messages in thread
From: Edgar Kaziakhmedov @ 2018-01-10 15:13 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, den
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 socket. But this approach
doesn't cover scenario when socket was closed while handshaking.
Therefore, if G_IO_IN is caught and qio_channel_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:
1) 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
Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
---
io/channel-websock.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 87ebdebfc0..384c34b390 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -81,6 +81,11 @@
QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
"Connection: close\r\n" \
"\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF \
+ "HTTP/1.1 403 Request Entity End Of File\r\n" \
+ QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
+ "Connection: close\r\n" \
+ "\r\n"
#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
#define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"
@@ -502,9 +507,14 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
error_setg(errp,
"End of headers not found in first 4096 bytes");
return 1;
- } else {
- return 0;
+ } else if (ret == 0) {
+ qio_channel_websock_handshake_send_res_err(
+ ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF);
+ error_setg(errp,
+ "End of headers not found before connection closed");
+ return -1;
}
+ return 0;
}
*handshake_end = '\0';
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data
2018-01-10 15:13 [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data Edgar Kaziakhmedov
@ 2018-01-10 15:22 ` Daniel P. Berrange
2018-01-10 15:24 ` Edgar Kaziakhmedov
0 siblings, 1 reply; 4+ messages in thread
From: Daniel P. Berrange @ 2018-01-10 15:22 UTC (permalink / raw)
To: Edgar Kaziakhmedov; +Cc: qemu-devel, den
On Wed, Jan 10, 2018 at 06:13:22PM +0300, Edgar Kaziakhmedov wrote:
> 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 socket. But this approach
> doesn't cover scenario when socket was closed while handshaking.
> Therefore, if G_IO_IN is caught and qio_channel_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:
> 1) 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
>
> Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
> ---
> io/channel-websock.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 87ebdebfc0..384c34b390 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -81,6 +81,11 @@
> QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
> "Connection: close\r\n" \
> "\r\n"
> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF \
> + "HTTP/1.1 403 Request Entity End Of File\r\n" \
> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
> + "Connection: close\r\n" \
> + "\r\n"
> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
> #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"
> @@ -502,9 +507,14 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
> error_setg(errp,
> "End of headers not found in first 4096 bytes");
> return 1;
> - } else {
> - return 0;
> + } else if (ret == 0) {
> + qio_channel_websock_handshake_send_res_err(
> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF);
> + error_setg(errp,
> + "End of headers not found before connection closed");
> + return -1;
Opps, my suggestion was slightly flawed - if we return -1, then we don't
need to call qio_channel_websock_handshake_send_res_err(), since we'll
just close the connection immediately.
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 :|
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data
2018-01-10 15:22 ` Daniel P. Berrange
@ 2018-01-10 15:24 ` Edgar Kaziakhmedov
2018-01-10 15:26 ` Daniel P. Berrange
0 siblings, 1 reply; 4+ messages in thread
From: Edgar Kaziakhmedov @ 2018-01-10 15:24 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, den
On 01/10/2018 06:22 PM, Daniel P. Berrange wrote:
> On Wed, Jan 10, 2018 at 06:13:22PM +0300, Edgar Kaziakhmedov wrote:
>> 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 socket. But this approach
>> doesn't cover scenario when socket was closed while handshaking.
>> Therefore, if G_IO_IN is caught and qio_channel_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:
>> 1) 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
>>
>> Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
>> ---
>> io/channel-websock.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/io/channel-websock.c b/io/channel-websock.c
>> index 87ebdebfc0..384c34b390 100644
>> --- a/io/channel-websock.c
>> +++ b/io/channel-websock.c
>> @@ -81,6 +81,11 @@
>> QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
>> "Connection: close\r\n" \
>> "\r\n"
>> +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF \
>> + "HTTP/1.1 403 Request Entity End Of File\r\n" \
>> + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
>> + "Connection: close\r\n" \
>> + "\r\n"
Well, don't you think that such http error code is not informative?
>> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
>> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
>> #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"
>> @@ -502,9 +507,14 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
>> error_setg(errp,
>> "End of headers not found in first 4096 bytes");
>> return 1;
>> - } else {
>> - return 0;
>> + } else if (ret == 0) {
>> + qio_channel_websock_handshake_send_res_err(
>> + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF);
>> + error_setg(errp,
>> + "End of headers not found before connection closed");
>> + return -1;
> Opps, my suggestion was slightly flawed - if we return -1, then we don't
> need to call qio_channel_websock_handshake_send_res_err(), since we'll
> just close the connection immediately.
Fixed
>
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data
2018-01-10 15:24 ` Edgar Kaziakhmedov
@ 2018-01-10 15:26 ` Daniel P. Berrange
0 siblings, 0 replies; 4+ messages in thread
From: Daniel P. Berrange @ 2018-01-10 15:26 UTC (permalink / raw)
To: Edgar Kaziakhmedov; +Cc: qemu-devel, den
On Wed, Jan 10, 2018 at 06:24:25PM +0300, Edgar Kaziakhmedov wrote:
>
>
> On 01/10/2018 06:22 PM, Daniel P. Berrange wrote:
> > On Wed, Jan 10, 2018 at 06:13:22PM +0300, Edgar Kaziakhmedov wrote:
> > > 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 socket. But this approach
> > > doesn't cover scenario when socket was closed while handshaking.
> > > Therefore, if G_IO_IN is caught and qio_channel_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:
> > > 1) 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
> > >
> > > Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
> > > ---
> > > io/channel-websock.c | 14 ++++++++++++--
> > > 1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > > index 87ebdebfc0..384c34b390 100644
> > > --- a/io/channel-websock.c
> > > +++ b/io/channel-websock.c
> > > @@ -81,6 +81,11 @@
> > > QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
> > > "Connection: close\r\n" \
> > > "\r\n"
> > > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF \
> > > + "HTTP/1.1 403 Request Entity End Of File\r\n" \
> > > + QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
> > > + "Connection: close\r\n" \
> > > + "\r\n"
> Well, don't you think that such http error code is not informative?
The only way they could ever see that is if they called shutdown() on the
socket to close the client->server data path, while leaving the server->client
data path open. That's so unlikely in context of HTTP clients that I don't
think its worth considering.
> > > #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
> > > #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
> > > #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"
> > > @@ -502,9 +507,14 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
> > > error_setg(errp,
> > > "End of headers not found in first 4096 bytes");
> > > return 1;
> > > - } else {
> > > - return 0;
> > > + } else if (ret == 0) {
> > > + qio_channel_websock_handshake_send_res_err(
> > > + ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_EOF);
> > > + error_setg(errp,
> > > + "End of headers not found before connection closed");
> > > + return -1;
> > Opps, my suggestion was slightly flawed - if we return -1, then we don't
> > need to call qio_channel_websock_handshake_send_res_err(), since we'll
> > just close the connection immediately.
> Fixed
> >
> >
> > Regards,
> > Daniel
>
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 :|
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-10 15:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-10 15:13 [Qemu-devel] [PATCH v2 1/1] io/channel-websock: handle continuous reads without any data Edgar Kaziakhmedov
2018-01-10 15:22 ` Daniel P. Berrange
2018-01-10 15:24 ` Edgar Kaziakhmedov
2018-01-10 15:26 ` Daniel P. Berrange
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).