[PATCH xserver 2/5] os: Use CloseDownFileDescriptor from AbortClient, including ospoll_remove

[Keith Packard]

AbortClient performs most of the same operations as
CloseDownFileDescriptor except that it doesn't call ospoll_remove,
leaving that unaware that the file descriptor has been closed.

If the file descriptor is re-used before the server comes back around
to clean up, and that new file descriptor is passed to SetNotifyFd,
then that function will mistakenly re-interpret the stale ClientPtr
returned by ospoll_data as a struct notify * instead and mangle data
badly.

To fix this, the patch does:

1) Change CloseDownFileDescriptor so that it can be called multiple
   times on the same OsCommPtr. The calls related to the file
   descriptor are moved inside the check for trans_conn and
   oc->trans_conn is set to NULL after cleaning up.

2) Move the XdmcpCloseDisplay call into CloseDownFileDescriptor. I
   don't think the actually matters as we just need to know at some
   point that the session client has exited. Moving it avoids the
   possibility of having this accidentally trigger from another client
   with the same fd which closes down at around the same time.

3) Change AbortClient to call CloseDownFileDescriptor. This makes sure
   that all of the fd-related clean up happens in the same way
   everywhere, in particular ensures that ospoll is notified about the
   closed file descriptor at the time it is closed and not some time later.

Debian-bug: https://bugs.debian.org/862824
Signed-off-by: Keith Packard <keithp at keithp.com>
---
 os/connection.c | 16 ++++++++--------
 os/io.c         |  4 +---
 os/osdep.h      |  3 +++
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/os/connection.c b/os/connection.c
index 66cb61070..a45a252a5 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -757,19 +757,22 @@ ErrorConnMax(XtransConnInfo trans_conn)
 
 /************
  *   CloseDownFileDescriptor:
- *     Remove this file descriptor and it's I/O buffers, etc.
+ *     Remove this file descriptor
  ************/
 
-static void
+void
 CloseDownFileDescriptor(OsCommPtr oc)
 {
-    int connection = oc->fd;
-
     if (oc->trans_conn) {
+        int connection = oc->fd;
+#ifdef XDMCP
+        XdmcpCloseDisplay(connection);
+#endif
+        ospoll_remove(server_poll, connection);
         _XSERVTransDisconnect(oc->trans_conn);
         _XSERVTransClose(oc->trans_conn);
+        oc->trans_conn = NULL;
     }
-    ospoll_remove(server_poll, connection);
 }
 
 /*****************
@@ -787,9 +790,6 @@ CloseDownConnection(ClientPtr client)
 
     if (oc->output)
 	FlushClient(client, oc, (char *) NULL, 0);
-#ifdef XDMCP
-    XdmcpCloseDisplay(oc->fd);
-#endif
     CloseDownFileDescriptor(oc);
     FreeOsBuffers(oc);
     free(client->osPrivate);
diff --git a/os/io.c b/os/io.c
index e1b90d17e..6a486613c 100644
--- a/os/io.c
+++ b/os/io.c
@@ -647,9 +647,7 @@ AbortClient(ClientPtr client)
     OsCommPtr oc = client->osPrivate;
 
     if (oc->trans_conn) {
-        _XSERVTransDisconnect(oc->trans_conn);
-        _XSERVTransClose(oc->trans_conn);
-        oc->trans_conn = NULL;
+        CloseDownFileDescriptor(oc);
         mark_client_ready(client);
     }
 }
diff --git a/os/osdep.h b/os/osdep.h
index 5545dd6bd..f6ce79b47 100644
--- a/os/osdep.h
+++ b/os/osdep.h
@@ -140,6 +140,9 @@ extern int FlushClient(ClientPtr /*who */ ,
 extern void FreeOsBuffers(OsCommPtr     /*oc */
     );
 
+void
+CloseDownFileDescriptor(OsCommPtr oc);
+
 #include "dix.h"
 #include "ospoll.h"
 
-- 
2.11.0