qemu/chardev
Daniel P. Berrangé 038b421788 Revert "chardev: use a child source for qio input source"
This reverts commit a7077b8e35,
and add comments to explain why child sources cannot be used.

When a GSource is added as a child of another GSource, if its
'prepare' function indicates readiness, then the parent's
'prepare' function will never be run. The io_watch_poll_prepare
absolutely *must* be run on every iteration of the main loop,
to ensure that the chardev backend doesn't feed data to the
frontend that it is unable to consume.

At the time a7077b8e35 was made,
all the child GSource impls were relying on poll'ing an FD,
so their 'prepare' functions would never indicate readiness
ahead of poll() being invoked. So the buggy behaviour was
not noticed and lay dormant.

Relatively recently the QIOChannelTLS impl introduced a
level 2 child GSource, which checks with GNUTLS whether it
has cached any data that was decoded but not yet consumed:

  commit ffda5db65a
  Author: Antoine Damhet <antoine.damhet@shadow.tech>
  Date:   Tue Nov 15 15:23:29 2022 +0100

    io/channel-tls: fix handling of bigger read buffers

    Since the TLS backend can read more data from the underlying QIOChannel
    we introduce a minimal child GSource to notify if we still have more
    data available to be read.

    Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
    Signed-off-by: Charles Frey <charles.frey@shadow.tech>
    Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

With this, it is now quite common for the 'prepare' function
on a QIOChannelTLS GSource to indicate immediate readiness,
bypassing the parent GSource 'prepare' function. IOW, the
critical 'io_watch_poll_prepare' is being skipped on some
iterations of the main loop. As a result chardev frontend
asserts are now being triggered as they are fed data they
are not ready to consume.

A reproducer is as follows:

 * In terminal 1 run a GNUTLS *echo* server

   $ gnutls-serv --echo \
                 --x509cafile ca-cert.pem \
                 --x509keyfile server-key.pem \
		 --x509certfile server-cert.pem \
		 -p 9000

 * In terminal 2 run a QEMU guest

   $ qemu-system-s390x \
       -nodefaults \
       -display none \
       -object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \
       -chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \
       -device sclpconsole,chardev=con0 \
       -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2

After the previous patch revert, but before this patch revert,
this scenario will crash:

  qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion
  `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.

This assert indicates that 'tcp_chr_read' was called without
'tcp_chr_read_poll' having first been checked for ability to
receive more data

QEMU's use of a 'prepare' function to create/delete another
GSource is rather a hack and not normally the kind of thing that
is expected to be done by a GSource. There is no mechanism to
force GLib to always run the 'prepare' function of a parent
GSource. The best option is to simply not use the child source
concept, and go back to the functional approach previously
relied on.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2024-03-19 20:17:12 +00:00
..
baum.c replace TABs with spaces 2023-03-20 12:43:50 +01:00
char-console.c
char-fd.c Refactoring: refactor TFR() macro to RETRY_ON_EINTR() 2023-01-09 13:50:47 +01:00
char-fe.c char: Slightly better error reporting when chardev is in use 2024-03-09 18:56:37 +03:00
char-file.c chardev: Allow setting file chardev input file on the command line 2023-04-20 06:50:11 +02:00
char-hmp-cmds.c char: Move HMP commands from monitor/ to chardev/ 2023-02-04 07:56:54 +01:00
char-io.c Revert "chardev: use a child source for qio input source" 2024-03-19 20:17:12 +00:00
char-mux.c chardev: don't exit() straight away on C-a x 2021-11-04 10:32:01 +00:00
char-null.c
char-parallel.c chardev/parallel: Don't close stdin on inappropriate device 2024-02-14 07:44:38 +01:00
char-pipe.c Refactoring: refactor TFR() macro to RETRY_ON_EINTR() 2023-01-09 13:50:47 +01:00
char-pty.c chardev/char-pty: Avoid losing bytes when the other side just (re-)connected 2023-10-03 15:40:09 +04:00
char-ringbuf.c Use DECLARE_*CHECKER* macros 2020-09-09 09:27:09 -04:00
char-serial.c chardev: replace qemu_set_nonblock() 2022-05-03 15:51:52 +04:00
char-socket.c Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend" 2024-03-19 20:17:12 +00:00
char-stdio.c chardev: replace qemu_set_nonblock() 2022-05-03 15:51:52 +04:00
char-udp.c qapi chardev: Elide redundant has_FOO in generated C 2022-12-14 20:04:47 +01:00
char-win-stdio.c chardev/char-win-stdio: Support VT sequences on Windows 11 host 2023-06-27 17:08:56 +02:00
char-win.c chardev: Improve error report by calling error_setg_win32() 2020-03-09 13:36:15 +01:00
char.c chardev: use bool for fe_is_open 2024-01-12 13:23:48 +00:00
chardev-internal.h Clean up decorations and whitespace around header guards 2022-05-11 16:50:32 +02:00
meson.build chardev/parallel: Don't close stdin on inappropriate device 2024-02-14 07:44:38 +01:00
msmouse.c ui/input: Constify QemuInputHandler structure 2023-10-19 23:13:28 +02:00
spice.c ui/spice: Require spice-server >= 0.14.0 2023-01-19 13:30:01 +01:00
testdev.c Use DECLARE_*CHECKER* macros 2020-09-09 09:27:09 -04:00
trace-events docs: fix references to docs/devel/tracing.rst 2021-06-02 06:51:09 +02:00
trace.h trace: switch position of headers to what Meson requires 2020-08-21 06:18:24 -04:00
wctablet.c ui/input: Constify QemuInputHandler structure 2023-10-19 23:13:28 +02:00