Rework composite overlay window code to fix several resource management bugs.

The composite overlay window code had several misunderstandings of the
workings of the X server, in particular error handling paths would often
double-free objects. Clean all of this up by using resource destruction as
the sole mechanism for freeing resource-based objects.

5 files changed, 206 insertions, 184 deletions

diff --git a/composite/Makefile.am b/composite/Makefile.am
index 21504e659..d7bead137 100644
--- a/composite/Makefile.am
+++ b/composite/Makefile.am
@@ -7,4 +7,5 @@ libcomposite_la_SOURCES = 	\
 	compext.c		\
 	compint.h		\
 	compinit.c		\
+	compoverlay.c		\
 	compwindow.c		
diff --git a/composite/compext.c b/composite/compext.c
index b3433f72b..e720f6ce7 100644
--- a/composite/compext.c
+++ b/composite/compext.c
@@ -54,10 +54,7 @@ static CARD8	CompositeReqCode;
 static DevPrivateKey CompositeClientPrivateKey = &CompositeClientPrivateKey;
 RESTYPE		CompositeClientWindowType;
 RESTYPE		CompositeClientSubwindowsType;
-static RESTYPE	CompositeClientOverlayType;
-
-static void deleteCompOverlayClient (CompOverlayClientPtr pOcToDel, 
-				     ScreenPtr pScreen);
+RESTYPE		CompositeClientOverlayType;
 
 typedef struct _CompositeClient {
     int	    major_version;
@@ -107,19 +104,8 @@ static int
 FreeCompositeClientOverlay (pointer value, XID ccwid)
 {
     CompOverlayClientPtr pOc = (CompOverlayClientPtr) value;
-    ScreenPtr pScreen = pOc->pScreen;
-    CompScreenPtr cs;
-
-    deleteCompOverlayClient(pOc, pScreen);
-
-    /* Unmap overlay window when there are no more clients using it */
-    cs = GetCompScreen(pScreen);
-    if (cs->pOverlayClients == NULL) {
-	if (cs->pOverlayWin != NULL) {
-	    UnmapWindow(cs->pOverlayWin, FALSE);
-	}
-    }
 
+    compFreeOverlayClient (pOc);
     return Success;
 }
 
@@ -304,137 +290,6 @@ ProcCompositeNameWindowPixmap (ClientPtr client)
 }
 
 
-/*
- * Routines for manipulating the per-screen overlay clients list.
- * This list indicates which clients have called GetOverlayWindow
- * for this screen.
- */
-
-/* Return the screen's overlay client list element for the given client */
-static CompOverlayClientPtr
-findCompOverlayClient (ClientPtr pClient, ScreenPtr pScreen) 
-{
-    CompScreenPtr    cs = GetCompScreen(pScreen);
-    CompOverlayClientPtr pOc;
-
-    for (pOc = cs->pOverlayClients; pOc != NULL; pOc = pOc->pNext) {
-	if (pOc->pClient == pClient) {
-	    return pOc;
-	}
-    }
-
-    return NULL;           
-}
-
-static int
-createCompOverlayClient (ClientPtr pClient, ScreenPtr pScreen)
-{
-    CompScreenPtr    cs = GetCompScreen(pScreen);
-    CompOverlayClientPtr pOc;
-
-    pOc = (CompOverlayClientPtr) xalloc(sizeof(CompOverlayClientRec));
-    if (pOc == NULL) {
-	return BadAlloc;
-    }
-    pOc->pClient = pClient;
-    pOc->pScreen = pScreen;
-    pOc->resource = FakeClientID(pClient->index);
-    pOc->pNext = cs->pOverlayClients;
-    cs->pOverlayClients = pOc;
-
-    /* 
-     * Create a resource for this element so it can be deleted
-     * when the client goes away.
-     */
-    if (!AddResource (pOc->resource, CompositeClientOverlayType, 
-		      (pointer) pOc)) {
-	xfree(pOc);
-	return BadAlloc;
-    }
-
-    return Success;
-}
-
-/* 
- * Delete the given overlay client list element from its screen list.
- */
-static void
-deleteCompOverlayClient (CompOverlayClientPtr pOcToDel, ScreenPtr pScreen)
-{
-    CompScreenPtr    cs = GetCompScreen(pScreen);
-    CompOverlayClientPtr pOc, pNext;
-    CompOverlayClientPtr pOcLast = NULL;
-
-    pOc = cs->pOverlayClients;
-    while (pOc != NULL) {
-	pNext = pOc->pNext;
-	if (pOc == pOcToDel) {
-	    xfree(pOc);
-	    if (pOcLast == NULL) {
-		cs->pOverlayClients = pNext;
-	    } else {
-		pOcLast->pNext = pNext;
-	    }
-	    break;
-	}
-	pOcLast = pOc;
-	pOc = pNext;
-    }
-}
-
-/* 
- * Delete all the hide-counts list elements for this screen.
- */
-void
-deleteCompOverlayClientsForScreen (ScreenPtr pScreen)
-{
-    CompScreenPtr    cs = GetCompScreen(pScreen);
-    CompOverlayClientPtr pOc, pTmp;
-
-    pOc = cs->pOverlayClients;
-    while (pOc != NULL) {
-	pTmp = pOc->pNext;
-	FreeResource(pOc->resource, 0);
-	pOc = pTmp;
-    }
-    cs->pOverlayClients = NULL;
-}
-
-/* 
-** If necessary, create the overlay window. And map it 
-** Note: I found it excessively difficult to destroy this window
-** during compCloseScreen; DeleteWindow can't be called because
-** the input devices are already shut down. So we are going to 
-** just allocate an overlay window once per screen per X server
-** invocation.
-*/
-
-static WindowPtr
-createOverlayWindow (ScreenPtr pScreen)
-{
-    int wid = FakeClientID(0);
-    WindowPtr pWin;
-    XID overrideRedirect = TRUE;
-    int result;
-
-    pWin = CreateWindow (
-	        wid, WindowTable[pScreen->myNum],
-    	        0, 0, pScreen->width, pScreen->height, 0, 
-	        InputOutput, CWOverrideRedirect, &overrideRedirect,
-	        WindowTable[pScreen->myNum]->drawable.depth, 
-	        serverClient, pScreen->rootVisual, &result);
-    if (pWin == NULL) {
-	return NULL;
-    }
-
-    if (!AddResource(wid, RT_WINDOW, (pointer)pWin)) {
-	DeleteWindow(pWin, None);
-	return NULL;
-    }
-
-    return pWin;
-}
-
 static int
 ProcCompositeGetOverlayWindow (ClientPtr client)
 {
@@ -456,28 +311,31 @@ ProcCompositeGetOverlayWindow (ClientPtr client)
     }
     pScreen = pWin->drawable.pScreen;
 
+    /* 
+     * Create an OverlayClient structure to mark this client's
+     * interest in the overlay window
+     */
+    pOc = compCreateOverlayClient(pScreen, client);
+    if (pOc == NULL)
+	return BadAlloc;
+
+    /*
+     * Make sure the overlay window exists
+     */
     cs = GetCompScreen(pScreen);
-    if (cs->pOverlayWin == NULL) {
-	cs->pOverlayWin = createOverlayWindow(pScreen);
-	if (cs->pOverlayWin == NULL) {
+    if (cs->pOverlayWin == NULL)
+	if (!compCreateOverlayWindow(pScreen))
+	{
+	    FreeResource (pOc->resource, RT_NONE);
 	    return BadAlloc;
 	}
-    }
 
     rc = XaceHook(XACE_RESOURCE_ACCESS, client, cs->pOverlayWin->drawable.id,
 		  RT_WINDOW, cs->pOverlayWin, RT_NONE, NULL, DixGetAttrAccess);
     if (rc != Success)
+    {
+	FreeResource (pOc->resource, RT_NONE);
 	return rc;
-
-    MapWindow(cs->pOverlayWin, serverClient);
-
-    /* Record that client is using this overlay window */
-    pOc = findCompOverlayClient(client, pScreen);
-    if (pOc == NULL) {
-	int ret = createCompOverlayClient(client, pScreen);
-	if (ret != Success) {
-	    return ret;
-	}
     }
 
     rep.type = X_Reply;
@@ -504,7 +362,6 @@ ProcCompositeReleaseOverlayWindow (ClientPtr client)
     WindowPtr pWin;
     ScreenPtr pScreen;
     CompOverlayClientPtr pOc;
-    CompScreenPtr cs;
 
     REQUEST_SIZE_MATCH(xCompositeReleaseOverlayWindowReq);
     pWin = (WindowPtr) LookupIDByType (stuff->window, RT_WINDOW);
@@ -519,18 +376,12 @@ ProcCompositeReleaseOverlayWindow (ClientPtr client)
      * Has client queried a reference to the overlay window
      * on this screen? If not, generate an error.
      */
-    pOc = findCompOverlayClient(client, pWin->drawable.pScreen);
-    if (pOc == NULL) {
+    pOc = compFindOverlayClient (pWin->drawable.pScreen, client);
+    if (pOc == NULL)
 	return BadMatch;
-    }
 
     /* The delete function will free the client structure */
-    FreeResource (pOc->resource, 0);
-
-    cs = GetCompScreen(pScreen);
-    if (cs->pOverlayClients == NULL) {
-	UnmapWindow(cs->pOverlayWin, FALSE);
-    }
+    FreeResource (pOc->resource, RT_NONE);
 
     return client->noClientException;
 }
diff --git a/composite/compinit.c b/composite/compinit.c
index 49b2044b0..7914a8d25 100644
--- a/composite/compinit.c
+++ b/composite/compinit.c
@@ -76,14 +76,6 @@ compCloseScreen (int index, ScreenPtr pScreen)
     pScreen->CopyWindow = cs->CopyWindow;
     pScreen->PositionWindow = cs->PositionWindow;
 
-    deleteCompOverlayClientsForScreen(pScreen);
-
-    /* 
-    ** Note: no need to call DeleteWindow; the server has
-    ** already destroyed it.
-    */
-    cs->pOverlayWin = NULL;
-
     xfree (cs);
     dixSetPrivate(&pScreen->devPrivates, CompScreenPrivateKey, NULL);
     ret = (*pScreen->CloseScreen) (index, pScreen);
@@ -122,11 +114,11 @@ compChangeWindowAttributes(WindowPtr pWin, unsigned long mask)
     if (ret && (mask & CWBackingStore)) {
 	if (pWin->backingStore != NotUseful) {
 	    compRedirectWindow(serverClient, pWin, CompositeRedirectAutomatic);
-	    pWin->backStorage = TRUE;
+	    pWin->backStorage = (pointer) (intptr_t) 1;
 	} else {
 	    compUnredirectWindow(serverClient, pWin,
 				 CompositeRedirectAutomatic);
-	    pWin->backStorage = FALSE;
+	    pWin->backStorage = NULL;
 	}
     }
 
@@ -380,6 +372,7 @@ compScreenInit (ScreenPtr pScreen)
 	return FALSE;
 
     cs->damaged = FALSE;
+    cs->overlayWid = FakeClientID(0);
     cs->pOverlayWin = NULL;
     cs->pOverlayClients = NULL;
 
diff --git a/composite/compint.h b/composite/compint.h
index 4b0fe0834..1c19ccd39 100644
--- a/composite/compint.h
+++ b/composite/compint.h
@@ -155,6 +155,7 @@ typedef struct _CompScreen {
     VisualID			*alternateVisuals;
 
     WindowPtr                   pOverlayWin;
+    Window			overlayWid;
     CompOverlayClientPtr        pOverlayClients;
     
 } CompScreenRec, *CompScreenPtr;
@@ -172,6 +173,7 @@ extern DevPrivateKey CompSubwindowsPrivateKey;
 
 extern RESTYPE		CompositeClientWindowType;
 extern RESTYPE		CompositeClientSubwindowsType;
+extern RESTYPE		CompositeClientOverlayType;
 
 /*
  * compalloc.c
@@ -230,6 +232,25 @@ Bool
 compScreenInit (ScreenPtr pScreen);
 
 /*
+ * compoverlay.c
+ */
+
+void
+compFreeOverlayClient (CompOverlayClientPtr pOcToDel);
+
+CompOverlayClientPtr
+compFindOverlayClient (ScreenPtr pScreen, ClientPtr pClient);
+    
+CompOverlayClientPtr
+compCreateOverlayClient (ScreenPtr pScreen, ClientPtr pClient);
+
+Bool
+compCreateOverlayWindow (ScreenPtr pScreen);
+
+void
+compDestroyOverlayWindow (ScreenPtr pScreen);
+
+/*
  * compwindow.c
  */
 
@@ -292,9 +313,6 @@ compCopyWindow (WindowPtr pWin, DDXPointRec ptOldOrg, RegionPtr prgnSrc);
 void
 compWindowUpdate (WindowPtr pWin);
 
-void
-deleteCompOverlayClientsForScreen (ScreenPtr pScreen);
-
 WindowPtr
 CompositeRealChildHead (WindowPtr pWin);
 
diff --git a/composite/compoverlay.c b/composite/compoverlay.c
new file mode 100644
index 000000000..94e5b0346
--- /dev/null
+++ b/composite/compoverlay.c
@@ -0,0 +1,159 @@
+/*
+ * Copyright © 2006 Sun Microsystems
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that
+ * copyright notice and this permission notice appear in supporting
+ * documentation, and that the name of Sun Microsystems not be used in
+ * advertising or publicity pertaining to distribution of the software without
+ * specific, written prior permission.  Sun Microsystems makes no
+ * representations about the suitability of this software for any purpose.  It
+ * is provided "as is" without express or implied warranty.
+ *
+ * SUN MICROSYSTEMS DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL SUN MICROSYSTEMS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+ * PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Copyright © 2003 Keith Packard
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that
+ * copyright notice and this permission notice appear in supporting
+ * documentation, and that the name of Keith Packard not be used in
+ * advertising or publicity pertaining to distribution of the software without
+ * specific, written prior permission.  Keith Packard makes no
+ * representations about the suitability of this software for any purpose.  It
+ * is provided "as is" without express or implied warranty.
+ *
+ * KEITH PACKARD DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL KEITH PACKARD BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+ * PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#ifdef HAVE_DIX_CONFIG_H
+#include <dix-config.h>
+#endif
+
+#include "compint.h"
+#include "xace.h"
+
+/* 
+ * Delete the given overlay client list element from its screen list.
+ */
+void
+compFreeOverlayClient (CompOverlayClientPtr pOcToDel)
+{
+    ScreenPtr		    pScreen = pOcToDel->pScreen;
+    CompScreenPtr	    cs = GetCompScreen (pScreen);
+    CompOverlayClientPtr    *pPrev, pOc;
+
+    for (pPrev = &cs->pOverlayClients; (pOc = *pPrev); pPrev = &pOc->pNext)
+    {
+	if (pOc == pOcToDel) {
+	    *pPrev = pOc->pNext;
+	    xfree (pOc);
+	    break;
+	}
+    }
+    
+    /* Destroy overlay window when there are no more clients using it */
+    if (cs->pOverlayClients == NULL)
+	compDestroyOverlayWindow (pScreen);
+}
+
+/*
+ * Return the client's first overlay client rec from the given screen
+ */
+CompOverlayClientPtr
+compFindOverlayClient (ScreenPtr pScreen, ClientPtr pClient)
+{
+    CompScreenPtr	    cs = GetCompScreen(pScreen);
+    CompOverlayClientPtr    pOc;
+
+    for (pOc = cs->pOverlayClients; pOc != NULL; pOc = pOc->pNext)
+	if (pOc->pClient == pClient)
+	    return pOc;
+
+    return NULL;           
+}
+
+/*
+ * Create an overlay client object for the given client
+ */
+CompOverlayClientPtr
+compCreateOverlayClient (ScreenPtr pScreen, ClientPtr pClient)
+{
+    CompScreenPtr    cs = GetCompScreen(pScreen);
+    CompOverlayClientPtr pOc;
+
+    pOc = (CompOverlayClientPtr) xalloc(sizeof(CompOverlayClientRec));
+    if (pOc == NULL)
+	return NULL;
+
+    pOc->pClient = pClient;
+    pOc->pScreen = pScreen;
+    pOc->resource = FakeClientID(pClient->index);
+    pOc->pNext = cs->pOverlayClients;
+    cs->pOverlayClients = pOc;
+
+    /* 
+     * Create a resource for this element so it can be deleted
+     * when the client goes away.
+     */
+    if (!AddResource (pOc->resource, CompositeClientOverlayType, (pointer) pOc))
+	return NULL;
+
+    return pOc;
+}
+
+/*
+ * Create the overlay window and map it
+ */
+Bool
+compCreateOverlayWindow (ScreenPtr pScreen)
+{
+    CompScreenPtr   cs = GetCompScreen(pScreen);
+    WindowPtr	    pRoot = WindowTable[pScreen->myNum];
+    WindowPtr	    pWin;
+    XID		    overrideRedirect = TRUE;
+    int		    result;
+
+    pWin = cs->pOverlayWin = 
+	CreateWindow (cs->overlayWid, pRoot,
+		      0, 0, pScreen->width, pScreen->height, 0, 
+		      InputOutput, CWOverrideRedirect, &overrideRedirect,
+		      pRoot->drawable.depth, 
+		      serverClient, pScreen->rootVisual, &result);
+    if (pWin == NULL)
+	return FALSE;
+
+    if (!AddResource(pWin->drawable.id, RT_WINDOW, (pointer)pWin))
+	return FALSE;
+
+    MapWindow(pWin, serverClient);
+
+    return TRUE;
+}
+
+/*
+ * Destroy the overlay window
+ */
+void
+compDestroyOverlayWindow (ScreenPtr pScreen)
+{
+    CompScreenPtr cs = GetCompScreen(pScreen);
+
+    cs->pOverlayWin = NullWindow;
+    FreeResource (cs->overlayWid, RT_NONE);
+}
+