os: Use CloseDownFileDescriptor from AbortClient, including ospoll_remove

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
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
This commit is contained in:
Keith Packard 2017-05-17 09:57:26 -07:00 committed by Adam Jackson
parent 5d941ccb0b
commit 523d35e3e1
3 changed files with 12 additions and 11 deletions

View File

@ -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);

View File

@ -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);
}
}

View File

@ -133,6 +133,9 @@ extern int FlushClient(ClientPtr /*who */ ,
extern void FreeOsBuffers(OsCommPtr /*oc */
);
void
CloseDownFileDescriptor(OsCommPtr oc);
#include "dix.h"
#include "ospoll.h"