[Qemu-devel] [PATCH] char: fix repeated registration of tcp chardev I/O handlers

[Qemu-devel] [PATCH] char: fix repeated registration of tcp chardev I/O handlers

[Daniel P. Berrange]

In previous commit:

  commit f2001a7e0555b66d6db25a3ff1801540814045bb
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Tue Jan 19 11:14:30 2016 +0000

    char: don't assume telnet initialization will not block

The code which writes the telnet initialization sequence moved
to an event loop callback. If the TCP chardev is opened as a
server in blocking mode (ie -serial telnet:0.0.0.0:3000,server,wait)
this results in a state where the TCP chardev is connected, but not
yet ready to send/recv data when virtual hardware is created.

When the virtual hardware initialization registers its chardev
callbacks, it triggers tcp_chr_update_read_handler, which will
add I/O watches to the connection.

When the telnet initialization finally runs, it will then call
tcp_chr_connect to finish the connection setup. This will in
turn add I/O watches to the connection too.

There are now two sets of I/O watches registered on the same
connection. This ultimately causes data loss on the connection,
for example, when typing into the telnet console only every
second byte is echoed back to the client.

The same flaw can affect channels running with TLS encryption
too, since they also have delayed connection setup completion.

The fix is to update tcp_chr_update_read_handler so that it
avoids registering watches if the connection is not fully
setup yet.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 927c47e..9060f8a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2856,6 +2856,10 @@ static void tcp_chr_update_read_handler(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
 
+    if (!s->connected) {
+        return;
+    }
+
     remove_fd_in_watch(chr);
     if (s->ioc) {
         chr->fd_in_tag = io_add_watch_poll(s->ioc,
-- 
2.5.0

Re: [Qemu-devel] [PATCH] char: fix repeated registration of tcp chardev I/O handlers

[Mark Cave-Ayland]

On 08/02/16 13:55, Daniel P. Berrange wrote:

> In previous commit:
> 
>   commit f2001a7e0555b66d6db25a3ff1801540814045bb
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Tue Jan 19 11:14:30 2016 +0000
> 
>     char: don't assume telnet initialization will not block
> 
> The code which writes the telnet initialization sequence moved
> to an event loop callback. If the TCP chardev is opened as a
> server in blocking mode (ie -serial telnet:0.0.0.0:3000,server,wait)
> this results in a state where the TCP chardev is connected, but not
> yet ready to send/recv data when virtual hardware is created.
> 
> When the virtual hardware initialization registers its chardev
> callbacks, it triggers tcp_chr_update_read_handler, which will
> add I/O watches to the connection.
> 
> When the telnet initialization finally runs, it will then call
> tcp_chr_connect to finish the connection setup. This will in
> turn add I/O watches to the connection too.
> 
> There are now two sets of I/O watches registered on the same
> connection. This ultimately causes data loss on the connection,
> for example, when typing into the telnet console only every
> second byte is echoed back to the client.
> 
> The same flaw can affect channels running with TLS encryption
> too, since they also have delayed connection setup completion.
> 
> The fix is to update tcp_chr_update_read_handler so that it
> avoids registering watches if the connection is not fully
> setup yet.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-char.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 927c47e..9060f8a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2856,6 +2856,10 @@ static void tcp_chr_update_read_handler(CharDriverState *chr)
>  {
>      TCPCharDriver *s = chr->opaque;
>  
> +    if (!s->connected) {
> +        return;
> +    }
> +
>      remove_fd_in_watch(chr);
>      if (s->ioc) {
>          chr->fd_in_tag = io_add_watch_poll(s->ioc,
> 

Hi Daniel,

Yes I can confirm that this patch fixes the issue for me - thanks a lot
for debugging!


ATB,

Mark.

Re: [Qemu-devel] [PATCH] char: fix repeated registration of tcp chardev I/O handlers

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt