[xserver] dix: Remove clients from input and output ready queues after closing

Submitted by Keith Packard on May 11, 2017, 2:18 p.m.

Details

Message ID 86fugbzdc5.fsf@hiro.keithp.com
State Superseded
Headers show
Series "dix: Remove clients from input and output ready queues after closing" ( rev: 2 ) in X.org (DEPRECATED - USE GITLAB)

Not browsing as part of any series.

Commit Message

Keith Packard May 11, 2017, 2:18 p.m. 
Michel Dänzer <michel@daenzer.net> writes:

> I'm afraid that doesn't explain the ClientReady-after-CloseDownClient
> case in https://bugs.freedesktop.org/attachment.cgi?id=131278 though, so
> there might still be another (possibly pre-existing) bug.

Agreed. I wonder if that might not be just flushing the set of
closed epoll data while there are still epoll entries for them. How
about this?

Patch hide | download patch | download mbox 

From b573c532223610cfc256ef5f6f7cee1512c7f383 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Thu, 11 May 2017 07:13:53 -0700
Subject: [PATCH xserver] os: Wait to clean deleted epoll entries until epoll
 is idle

Make sure there are no epoll entries in the kernel before
clearing the queue of deleted ones.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 os/ospoll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/os/ospoll.c b/os/ospoll.c
index 51bd02dc7..2fbfd6706 100644
--- a/os/ospoll.c
+++ b/os/ospoll.c
@@ -394,6 +394,8 @@  ospoll_wait(struct ospoll *ospoll, int timeout)
     struct epoll_event events[MAX_EVENTS];
     int i;
 
+    if (!xorg_list_is_empty(&ospoll->deleted))
+        timeout = 0;
     nready = epoll_wait(ospoll->epoll_fd, events, MAX_EVENTS, timeout);
     for (i = 0; i < nready; i++) {
         struct epoll_event *ev = &events[i];
@@ -411,7 +413,8 @@  ospoll_wait(struct ospoll *ospoll, int timeout)
         if (osfd->callback)
             osfd->callback(osfd->fd, xevents, osfd->data);
     }
-    ospoll_clean_deleted(ospoll);
+    if (nready == 0)
+        ospoll_clean_deleted(ospoll);
 #endif
 #if POLL
     nready = xserver_poll(ospoll->fds, ospoll->num, timeout);
-- 
2.11.0

Comments

Michel Dänzer May 12, 2017, 1:59 a.m. 
On 11/05/17 11:18 PM, Keith Packard wrote:
> Michel Dänzer <michel@daenzer.net> writes:
> 
>> I'm afraid that doesn't explain the ClientReady-after-CloseDownClient
>> case in https://bugs.freedesktop.org/attachment.cgi?id=131278 though, so
>> there might still be another (possibly pre-existing) bug.
> 
> Agreed. I wonder if that might not be just flushing the set of
> closed epoll data while there are still epoll entries for them. How
> about this?

Makes sense. Nick, can you try doing what you were doing to reproduce
the crashes with this applied, and check that valgrind doesn't report
anything like:

==9640== Invalid write of size 8
==9640==    at 0x159755: __xorg_list_add (list.h:135)
==9640==    by 0x159755: xorg_list_append (list.h:177)
==9640==    by 0x159755: mark_client_ready (dispatch.c:265)
==9640==    by 0x2A6007: ClientReady (connection.c:712)
==9640==    by 0x2AA0C0: ospoll_wait (ospoll.c:412)
==9640==    by 0x2A3413: WaitForSomething (WaitFor.c:226)
==9640==    by 0x15FCC1: Dispatch (dispatch.c:421)
==9640==    by 0x163D76: dix_main (main.c:276)
==9640==    by 0x6ABF1C0: (below main) (libc-start.c:289)
==9640==  Address 0x82ece30 is 16 bytes inside a block of size 360 free'd
==9640==    at 0x4C2C03B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9640==    by 0x15F333: CloseDownClient (dispatch.c:3484)
==9640==    by 0x2AA0C0: ospoll_wait (ospoll.c:412)
==9640==    by 0x2A3413: WaitForSomething (WaitFor.c:226)
==9640==    by 0x15FCC1: Dispatch (dispatch.c:421)
==9640==    by 0x163D76: dix_main (main.c:276)
==9640==    by 0x6ABF1C0: (below main) (libc-start.c:289)
[...]