From 4d4102f6e2f9afd6182888787ae8b570347df87d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 18 Mar 2024 18:06:59 +0000 Subject: [PATCH 1/3] chardev: lower priority of the HUP GSource in socket chardev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Daniel P. Berrangé RH-MergeRequest: 233: Fix handling of TLS sessions in chardevs RH-Jira: RHEL-24614 RH-Acked-by: Thomas Huth RH-Acked-by: Marc-André Lureau RH-Commit: [1/3] 842f54349191b0206e68f35a7a80155f5a584942 (berrange/centos-src-qemu) The socket chardev often has 2 GSource object registered against the same FD. One is registered all the time and is just intended to handle POLLHUP events, while the other gets registered & unregistered on the fly as the frontend is ready to receive more data or not. It is very common for poll() to signal a POLLHUP event at the same time as there is pending incoming data from the disconnected client. It is therefore essential to process incoming data prior to processing HUP. The problem with having 2 GSource on the same FD is that there is no guaranteed ordering of execution between them, so the chardev code may process HUP first and thus discard data. This failure scenario is non-deterministic but can be seen fairly reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and then running 'tests/unit/test-char', which will sometimes fail with missing data. Ideally QEMU would only have 1 GSource, but that's a complex code refactoring job. The next best solution is to try to ensure ordering between the 2 GSource objects. This can be achieved by lowering the priority of the HUP GSource, so that it is never dispatched if the main GSource is also ready to dispatch. Counter-intuitively, lowering the priority of a GSource is done by raising its priority number. Reviewed-by: Marc-André Lureau Reviewed-by: Thomas Huth Signed-off-by: Daniel P. Berrangé (cherry picked from commit 8bd8b04adc9f18904f323dff085f8b4ec77915c6) --- chardev/char-socket.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 034840593d..f48d341ebc 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -597,6 +597,22 @@ static void update_ioc_handlers(SocketChardev *s) remove_hup_source(s); s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP); + /* + * poll() is liable to return POLLHUP even when there is + * still incoming data available to read on the FD. If + * we have the hup_source at the same priority as the + * main io_add_watch_poll GSource, then we might end up + * processing the POLLHUP event first, closing the FD, + * and as a result silently discard data we should have + * read. + * + * By setting the hup_source to G_PRIORITY_DEFAULT + 1, + * we ensure that io_add_watch_poll GSource will always + * be dispatched first, thus guaranteeing we will be + * able to process all incoming data before closing the + * FD + */ + g_source_set_priority(s->hup_source, G_PRIORITY_DEFAULT + 1); g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup, chr, NULL); g_source_attach(s->hup_source, chr->gcontext); -- 2.39.3