From 52f11ad5c23d759024ac486542cced65ba760fc0 Mon Sep 17 00:00:00 2001
From: "Enrico Weigelt, metux IT consult" <info@metux.net>
Date: Wed, 26 Jun 2024 17:21:01 +0200
Subject: [PATCH] (submit/fix-SelectSelectionInput) dix: create empty selection
 objects as-needed in dixLookupSelection()

For now, new selection objects are only created in ProcSetSelectionOwner()
when dixLookupSelection() can't find the requested one (returns BadMatch).

When somebody's trying to listen on a not-yet existing selection, via
XFixesSelectSelectionInput() -- XFIXES:SelectSelectionInput message -- he's
also getting BadMatch. This isn't neccessarily completely wrong, the spec
doesn't really tell anything about those situations (it doens't tell anything
about selection's lifetimes, just their ownerships). But there are real-
world clients not expecting an error here and crashing - the problem popped
up just recently, due to a necessary security fix (remote memory corruption
plus XACE missing to catch SelectSelectionInput) that alread went unnoticed
for far too long (*1).

So, it's better being polite and interpret the spec in the way that any
potential selection exists as soon as it's used by someone. (in fact, they
never get deleted anyways, just cleared).

XACE consumers get properly notified by the new Selection object creation
(eg. SElinux is attaching it's private data to it). And all callers already
prepared to get a cleared Selection object, because that's always been a
perfectly normal situation - Selection objects never get removed again,
just cleared.

*1) 601fd0fd8446a4377180d9695b469a48aa352d71

X-Fixes: 601fd0fd8446a4377180d9695b469a48aa352d71
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 dix/selection.c | 72 +++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 41 deletions(-)

diff --git a/dix/selection.c b/dix/selection.c
index fa50ac671..fea7a911a 100644
--- a/dix/selection.c
+++ b/dix/selection.c
@@ -82,8 +82,20 @@ dixLookupSelection(Selection ** result, Atom selectionName,
         if (pSel->selection == selectionName)
             break;
 
-    if (pSel)
-        rc = XaceHookSelectionAccess(client, &pSel, access_mode);
+    if (!pSel) {
+        if (!(pSel = dixAllocateObjectWithPrivates(Selection, PRIVATE_SELECTION)))
+            return BadAlloc;
+        pSel->selection = selectionName;
+        pSel->next = CurrentSelections;
+        CurrentSelections = pSel;
+    }
+
+    /* security creation/labeling check */
+    rc = XaceHookSelectionAccess(client, &pSel, access_mode | DixCreateAccess);
+    if (rc != Success) {
+        return rc;
+    }
+
     *result = pSel;
     return rc;
 }
@@ -175,47 +187,25 @@ ProcSetSelectionOwner(ClientPtr client)
      */
     rc = dixLookupSelection(&pSel, stuff->selection, client, DixSetAttrAccess);
 
-    if (rc == Success) {
-        /* If the timestamp in client's request is in the past relative
-           to the time stamp indicating the last time the owner of the
-           selection was set, do not set the selection, just return
-           success. */
-        if (CompareTimeStamps(time, pSel->lastTimeChanged) == EARLIER)
-            return Success;
-        if (pSel->client && (!pWin || (pSel->client != client))) {
-            xEvent event = {
-                .u.selectionClear.time = time.milliseconds,
-                .u.selectionClear.window = pSel->window,
-                .u.selectionClear.atom = pSel->selection
-            };
-            event.u.u.type = SelectionClear;
-            WriteEventsToClient(pSel->client, 1, &event);
-        }
-    }
-    else if (rc == BadMatch) {
-        /*
-         * It doesn't exist, so add it...
-         */
-        pSel = dixAllocateObjectWithPrivates(Selection, PRIVATE_SELECTION);
-        if (!pSel)
-            return BadAlloc;
-
-        pSel->selection = stuff->selection;
-
-        /* security creation/labeling check */
-        rc = XaceHookSelectionAccess(client, &pSel,
-                                     DixCreateAccess | DixSetAttrAccess);
-        if (rc != Success) {
-            free(pSel);
-            return rc;
-        }
-
-        pSel->next = CurrentSelections;
-        CurrentSelections = pSel;
-    }
-    else
+    if (rc != Success)
         return rc;
 
+    /* If the timestamp in client's request is in the past relative
+       to the time stamp indicating the last time the owner of the
+       selection was set, do not set the selection, just return
+       success. */
+    if (CompareTimeStamps(time, pSel->lastTimeChanged) == EARLIER)
+        return Success;
+    if (pSel->client && (!pWin || (pSel->client != client))) {
+        xEvent event = {
+            .u.selectionClear.time = time.milliseconds,
+            .u.selectionClear.window = pSel->window,
+            .u.selectionClear.atom = pSel->selection
+        };
+        event.u.u.type = SelectionClear;
+        WriteEventsToClient(pSel->client, 1, &event);
+    }
+
     pSel->lastTimeChanged = time;
     pSel->window = stuff->window;
     pSel->pWin = pWin;