rootless: Fix SIGBUS when copying 8bit window contents upwards

glx/glxext.c

@@ -281,24 +281,6 @@ GlxPushProvider(__GLXprovider * provider)
     __glXProviderStack = provider;
 }
 
-static Bool
-checkScreenVisuals(void)
-{
-    int i, j;
-
-    for (i = 0; i < screenInfo.numScreens; i++) {
-        ScreenPtr screen = screenInfo.screens[i];
-        for (j = 0; j < screen->numVisuals; j++) {
-            if ((screen->visuals[j].class == TrueColor ||
-                 screen->visuals[j].class == DirectColor) &&
-                screen->visuals[j].nplanes > 12)
-                return TRUE;
-        }
-    }
-
-    return FALSE;
-}
-
 static void
 GetGLXDrawableBytes(void *value, XID id, ResourceSizePtr size)
 {
@@ -472,10 +454,6 @@ static Bool
 xorgGlxServerPreInit(const ExtensionEntry *extEntry)
 {
     if (glxGeneration != serverGeneration) {
-        /* Mesa requires at least one True/DirectColor visual */
-        if (!checkScreenVisuals())
-            return FALSE;
-
         __glXContextRes = CreateNewResourceType((DeleteType) ContextGone,
                                                 "GLXContext");
         __glXDrawableRes = CreateNewResourceType((DeleteType) DrawableGone,

hw/xquartz/xpr/xprFrame.c

@@ -106,11 +106,18 @@ xprCopyWindow(RootlessFrameID wid, int dstNrects, const BoxRec *dstRects,
               int dx,
               int dy);
 
-static inline xp_error
+static inline void
 xprConfigureWindow(xp_window_id id, unsigned int mask,
                    const xp_window_changes *values)
 {
-    return xp_configure_window(id, mask, values);
+    xp_error err = xp_configure_window(id, mask, values);
+
+    /* Log rather than drop a reconfigure silently: the implementation rejects every mask on a frame that is locked for
+     * drawing, which would otherwise leave a window on screen that the X server believes it has already moved, resized
+     * or unmapped.
+     */
+    if (err != Success)
+        ErrorF("Could not configure window %d, mask 0x%x (%d).\n", (int)id, mask, (int)err);
 }
 
 static void
@@ -197,9 +204,9 @@ xprCreateFrame(RootlessWindowPtr pFrame, ScreenPtr pScreen,
         return FALSE;
     }
 
-    dispatch_async(window_hash_serial_q, ^ {
-                       x_hash_table_insert(window_hash, pFrame->wid, pFrame);
-                   });
+    dispatch_sync(window_hash_serial_q, ^ {
+                      x_hash_table_insert(window_hash, pFrame->wid, pFrame);
+                  });
 
     xprSetNativeProperty(pFrame);
 
@@ -214,15 +221,15 @@ xprDestroyFrame(RootlessFrameID wid)
 {
     xp_error err;
 
-    dispatch_async(window_hash_serial_q, ^ {
-                       x_hash_table_remove(window_hash, wid);
-                   });
+    dispatch_sync(window_hash_serial_q, ^ {
+                      x_hash_table_remove(window_hash, wid);
+                  });
 
     err = xp_destroy_window(x_cvt_vptr_to_uint(wid));
+    /* Not a FatalError: leaking the window costs us far less than terminating every client connected to the server.
+     */
     if (err != Success)
-        FatalError("Could not destroy window %d (%d).",
-                   (int)x_cvt_vptr_to_uint(
-                       wid), (int)err);
+        ErrorF("Could not destroy window %d (%d).\n", (int)x_cvt_vptr_to_uint(wid), (int)err);
 }
 
 /*
@@ -284,6 +291,9 @@ xprRestackFrame(RootlessFrameID wid, RootlessFrameID nextWid)
         wc.sibling = x_cvt_vptr_to_uint(nextWid);
     }
 
+    /* Unlike xprGetXWindow(), reading the record after the critical section is safe here because this only ever runs on
+     * the server thread, which is also the only thread that frees it.
+     */
     dispatch_sync(window_hash_serial_q, ^ {
                       winRec = x_hash_table_lookup(window_hash, wid, NULL);
                   });
@@ -355,6 +365,10 @@ xprStartDrawing(RootlessFrameID wid, char **pixelData, int *bytesPerRow)
 
     err = xp_lock_window(x_cvt_vptr_to_uint(
                              wid), NULL, NULL, data, rowbytes, NULL);
+    /* Deliberately still fatal, unlike the failures either side of this: the implementation leaves data[] untouched on
+     * failure and RootlessStartDrawing() cannot refuse, so carrying on would hand fb an uninitialized pointer to draw
+     * through.  Demoting this needs a way to report the failure upwards first.
+     */
     if (err != Success)
         FatalError("Could not lock window %d for drawing (%d).",
                    (int)x_cvt_vptr_to_uint(
@@ -386,7 +400,7 @@ xprStopDrawing(RootlessFrameID wid, Bool flush)
      * FatalError after http://xquartz.macosforge.org/trac/ticket/482 is fixed.
      */
     if (err != Success)
-        ErrorF("Could not unlock window %d after drawing (%d).",
+        ErrorF("Could not unlock window %d after drawing (%d).\n",
                (int)x_cvt_vptr_to_uint(
                    wid), (int)err);
 }
@@ -476,10 +490,14 @@ xprInit(ScreenPtr pScreen)
     rootless_CopyBytes_threshold = xp_copy_bytes_threshold;
     rootless_CopyWindow_threshold = xp_scroll_area_threshold;
 
-    assert((window_hash = x_hash_table_new(NULL, NULL, NULL, NULL)));
-    assert((window_hash_serial_q =
-                dispatch_queue_create(BUNDLE_ID_PREFIX ".X11.xpr_window_hash",
-                                      NULL)));
+    window_hash = x_hash_table_new(NULL, NULL, NULL, NULL);
+    if (window_hash == NULL)
+        FatalError("Could not allocate window hash.");
+
+    window_hash_serial_q =
+        dispatch_queue_create(BUNDLE_ID_PREFIX ".X11.xpr_window_hash", NULL);
+    if (window_hash_serial_q == NULL)
+        FatalError("Could not create window hash queue.");
 
     return TRUE;
 }
@@ -491,14 +509,21 @@ xprInit(ScreenPtr pScreen)
 WindowPtr
 xprGetXWindow(xp_window_id wid)
 {
-    RootlessWindowRec *winRec __block;
+    WindowPtr pWin __block = NULL;
+
+    /* Load the window inside the critical section.  Letting the frame record escape it would defeat the point, since
+     * the server thread frees the record as soon as the frame is destroyed.
+     */
     dispatch_sync(window_hash_serial_q, ^ {
-                      winRec =
+                      RootlessWindowRec *winRec =
                           x_hash_table_lookup(window_hash,
                                               x_cvt_uint_to_vptr(wid), NULL);
+
+                      if (winRec != NULL)
+                          pWin = winRec->win;
                   });
 
-    return winRec != NULL ? winRec->win : NULL;
+    return pWin;
 }
 
 /*
@@ -566,14 +591,16 @@ xprHideWindows(Bool hide)
 
 Bool no_configure_window;
 
-static inline int
+/* As xprConfigureWindow(), but honoring no_configure_window so that responding to a native window change does not echo
+ * straight back to the implementation.  Only the colormap and hide paths below go through this; the move, resize,
+ * restack, reshape and unmap hooks call xprConfigureWindow() directly and so are not suppressed.
+ */
+static inline void
 configure_window(xp_window_id id, unsigned int mask,
                  const xp_window_changes *values)
 {
     if (!no_configure_window)
-        return xp_configure_window(id, mask, values);
-    else
-        return XP_Success;
+        xprConfigureWindow(id, mask, values);
 }
 
 static

miext/rootless/README.txt

@@ -173,7 +173,8 @@ typedef Bool (*RootlessCreateFrameProc)
 
 /*
  * Destroy a frame.
- *  Drawing is stopped and all updates are flushed before this is called.
+ *  Drawing is stopped before this is called.
+ *  The frame record stays valid for the duration of the call and is freed immediately afterwards.
  *
  *  wid         Frame id
  */
@@ -182,7 +183,7 @@ typedef void (*RootlessDestroyFrameProc)
 
 /*
  * Move a frame on screen.
- *  Drawing is stopped and all updates are flushed before this is called.
+ *  Drawing is stopped before this is called.
  *
  *  wid         Frame id
  *  pScreen     Screen to move the new frame to
@@ -232,6 +233,7 @@ typedef void (*RootlessReshapeFrameProc)
 
 /*
  * Unmap a frame.
+ *  Drawing is stopped before this is called.
  *
  *  wid         Frame id
  */

miext/rootless/rootless.h

@@ -114,7 +114,8 @@ typedef Bool (*RootlessCreateFrameProc)
 
 /*
  * Destroy a frame.
- *  Drawing is stopped and all updates are flushed before this is called.
+ *  Drawing is stopped before this is called.
+ *  The frame record stays valid for the duration of the call and is freed immediately afterwards.
  *
  *  wid         Frame id
  */
@@ -123,7 +124,7 @@ typedef void (*RootlessDestroyFrameProc)
 
 /*
  * Move a frame on screen.
- *  Drawing is stopped and all updates are flushed before this is called.
+ *  Drawing is stopped before this is called.
  *
  *  wid         Frame id
  *  pScreen     Screen to move the new frame to
@@ -172,6 +173,7 @@ typedef void (*RootlessReshapeFrameProc)
 
 /*
  * Unmap a frame.
+ *  Drawing is stopped before this is called.
  *
  *  wid         Frame id
  */