From 4308f5d3d1fbd0f5dce81e22c0c3f08c65a7c9d8 Mon Sep 17 00:00:00 2001 From: Aaron Plattner Date: Tue, 19 Nov 2019 10:08:51 -0800 Subject: [PATCH] os: Don't crash in AttendClient if the client is gone If a client is in the process of being closed down, then its client->osPrivate pointer will be set to NULL by CloseDownConnection. This can cause a crash if freeing the client's resources results in a call to AttendClient. For example, if the client has a pending sync fence: Thread 1 "X" received signal SIGSEGV, Segmentation fault. AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942 (gdb) bt #0 AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942 #1 0x00005571c3dbb865 in SyncAwaitTriggerFired (pTrigger=) at ../Xext/sync.c:694 #2 0x00005571c3dd5749 in miSyncDestroyFence (pFence=0x5571c5063980) at ../miext/sync/misync.c:120 #3 0x00005571c3dbbc69 in FreeFence (obj=, id=) at ../Xext/sync.c:1909 #4 0x00005571c3d7a01d in doFreeResource (res=0x5571c506e3d0, skip=skip@entry=0) at ../dix/resource.c:880 #5 0x00005571c3d7b1dc in FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1146 #6 FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1109 #7 0x00005571c3d5525f in CloseDownClient (client=0x5571c4aed9a0) at ../dix/dispatch.c:3473 #8 0x00005571c3d55eeb in Dispatch () at ../dix/dispatch.c:492 #9 0x00005571c3d59e96 in dix_main (argc=3, argv=0x7ffe7854bc28, envp=) at ../dix/main.c:276 #10 0x00007fea4837cb6b in __libc_start_main (main=0x5571c3d1d060
, argc=3, argv=0x7ffe7854bc28, init=, fini=, rtld_fini=, stack_end=0x7ffe7854bc18) at ../csu/libc-start.c:308 #11 0x00005571c3d1d09a in _start () at ../Xext/sync.c:2378 (gdb) print client->osPrivate $1 = (void *) 0x0 Since the client is about to be freed, its ignore count doesn't matter and AttendClient can simply be a no-op. Check for client->clientGone in AttendClient and remove similar checks from two callers that had them. Signed-off-by: Aaron Plattner --- Xext/sleepuntil.c | 3 +-- dix/dixutils.c | 9 +-------- os/connection.c | 8 ++++++++ record/record.c | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Xext/sleepuntil.c b/Xext/sleepuntil.c index 68a7a9bee..334c2784a 100644 --- a/Xext/sleepuntil.c +++ b/Xext/sleepuntil.c @@ -125,8 +125,7 @@ ClientSleepUntil(ClientPtr client, static void ClientAwaken(ClientPtr client, void *closure) { - if (!client->clientGone) - AttendClient(client); + AttendClient(client); } static int diff --git a/dix/dixutils.c b/dix/dixutils.c index 2983174a1..331ccfb96 100644 --- a/dix/dixutils.c +++ b/dix/dixutils.c @@ -662,14 +662,7 @@ ClientWakeup(ClientPtr client) if (q->client == client) { *prev = q->next; free(q); - if (client->clientGone) - /* Oops -- new zombie cleanup code ensures this only - * happens from inside CloseDownClient; don't want to - * recurse here... - */ - /* CloseDownClient(client) */ ; - else - AttendClient(client); + AttendClient(client); break; } prev = &q->next; diff --git a/os/connection.c b/os/connection.c index a1c42aede..85e0050d8 100644 --- a/os/connection.c +++ b/os/connection.c @@ -924,6 +924,14 @@ AttendClient(ClientPtr client) { OsCommPtr oc = (OsCommPtr) client->osPrivate; + if (client->clientGone) { + /* + * client is gone, so any pending requests will be dropped and its + * ignore count doesn't matter. + */ + return; + } + client->ignoreCount--; if (client->ignoreCount) return; diff --git a/record/record.c b/record/record.c index 757fde1fe..42aee324a 100644 --- a/record/record.c +++ b/record/record.c @@ -2362,9 +2362,9 @@ RecordDisableContext(RecordContextPtr pContext) if (!pContext->pRecordingClient->clientGone) { RecordAProtocolElement(pContext, NULL, XRecordEndOfData, NULL, 0, 0, 0); RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0); - /* Re-enable request processing on this connection. */ - AttendClient(pContext->pRecordingClient); } + /* Re-enable request processing on this connection. */ + AttendClient(pContext->pRecordingClient); for (pRCAP = pContext->pListOfRCAP; pRCAP; pRCAP = pRCAP->pNextRCAP) { RecordUninstallHooks(pRCAP, 0);