Bug 1191 - Fixes for Kdrive composite exposures
Summary: Fixes for Kdrive composite exposures
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/DDX/Xephyr (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high critical
Assignee: Keith Packard
QA Contact:
Whiteboard: 2011BRB_Reviewed
Keywords: patch
Depends on:
Reported: 2004-08-26 05:52 UTC by Damien Ciabrini
Modified: 2018-06-11 18:12 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

proposed patch for fixing composite exposures (781 bytes, patch)
2004-08-26 05:57 UTC, Damien Ciabrini
no flags Details | Splinter Review
Patch that fixes the exposure bugs (781 bytes, patch)
2005-12-31 03:32 UTC, Damien Ciabrini
no flags Details | Splinter Review

Description Damien Ciabrini 2004-08-26 05:52:56 UTC
Kdrive crashes in the handling of window exposures when
xcompmgr is running (ie in composite code). 

To reproduce the bug, start afterstep and then xcompmgr. when both are
loaded, simply start afterstep's background manager:
  asetroot 0 0 -l

This make Kdrive SIGBUS when it tries to repaint the root window.

The problem comes from the fact that asetroot make a call to
XCreateWindow with negatives position (IIRC x=-10000, y=-10000).
The server then try to redisplay the whole screen starting from the root
window. It computes the exposed regions to redisplay. 

The SIGBUS comes from the fact that when "Redirect to pixmap" is enable
(ie xcompmgr is running), some exposed regions contains negative values.
Note that without xcompmgr (thus without redirect to pixmap) regions
never contains negatives and thus Kdrive never segfaults.

I proposed a patch on the xorg ML, it is a simple fix to avoid negatives 
values in such exposure regions. With this patch, the computation of 
exposed regions returns exactly the same result whether "Redirect to pixmap"
is enable or not.
Comment 1 Damien Ciabrini 2004-08-26 05:57:54 UTC
Created attachment 734 [details] [review]
proposed patch for fixing composite exposures

The patch that I proposed to the xorg ML some days ago.
Comment 2 Eric Anholt 2005-12-30 14:35:04 UTC
Comment on attachment 734 [details] [review]
proposed patch for fixing composite exposures

This patch is toast, from the disk crash I'd guess.  Reattach?
Comment 3 Damien Ciabrini 2005-12-31 03:32:49 UTC
Created attachment 4202 [details] [review]
Patch that fixes the exposure bugs

Here it is... anyway this patch is only an underflow check. anholt or keipth,
tell me if this helps:)
Comment 4 Adam Jackson 2009-04-14 07:11:06 UTC
Moving to xorg
Comment 5 Jeremy Huddleston Sequoia 2011-09-25 16:46:19 UTC
is this still relevant?  The patches are a tad old and probably need to be 
rebased.  If you are still having a problem, I suggest you update your patch 
and send it to xorg-devel for review by a wider audience.  If it's not a 
problem (or we don't hear back in a while), I'm just going to close this bug 
since kdrive will probably be gone in the next year or two anyway.
Comment 6 Adam Jackson 2018-06-11 18:12:48 UTC
The patch is gzipped, though apparently bz can't figure that out. It looks like this:

--- xserver/mi/mivaltree.c      2004-08-18 21:43:15.000000000 +0000
+++ xserver-new/mi/mivaltree.c  2004-08-18 21:47:14.000000000 +0000
@@ -275,6 +275,15 @@
        if (miSetRedirectBorderClipProc)
            (*miSetRedirectBorderClipProc) (pParent, universe);
        REGION_COPY(pScreen, universe, &pParent->borderSize);
+       /* 
+        * Check that universe does not contain negative values
+        * so that the result of miComputeClip stays valid.
+        */
+       if (universe->extents.x1 < 0) universe->extents.x1 = 0;
+       if (universe->extents.y1 < 0) universe->extents.y1 = 0;
+       if (universe->extents.x2 < 0) universe->extents.x2 = 0;
+       if (universe->extents.y2 < 0) universe->extents.y2 = 0;
--- xserver/composite/compwindow.c      2004-08-18 21:43:03.000000000 +0000
+++ xserver-new/composite/compwindow.c  2004-08-18 21:42:24.000000000 +0000
@@ -586,14 +586,17 @@
     CompWindowPtr   cw = GetCompWindow (pWin);
     RegionRec      damage;
+    int             translatex = pWin->drawable.x - cw->borderClipX;
+    int             translatey = pWin->drawable.y - cw->borderClipY;
     REGION_INIT (pScreen, &damage, NullBox, 0);
      * Align old border clip with new border clip
+     * Avoid negative values
     REGION_TRANSLATE (pScreen, &cw->borderClip,
-                     pWin->drawable.x - cw->borderClipX,
-                     pWin->drawable.y - cw->borderClipY);
+                     (translatex>0) ? translatex : 0,
+                     (translatey>0) ? translatey : 0);
      * Compute newly visible portion of window for repaint
@@ -604,11 +607,11 @@
     DamageDamageRegion (&pWin->drawable, &damage);
     REGION_UNINIT (pScreen, &damage);
-     * Save the new border clip region
+     * Save the new border clip region. Avoid negative values
     REGION_COPY (pScreen, &cw->borderClip, pRegion);
-    cw->borderClipX = pWin->drawable.x;
-    cw->borderClipY = pWin->drawable.y;
+    cw->borderClipX = (pWin->drawable.x > 0) ? pWin->drawable.x : 0;
+    cw->borderClipY = (pWin->drawable.y > 0) ? pWin->drawable.y : 0;

Obviously that doesn't apply to current git. More importantly the fix is wrong, regions are allowed to have negative coordinates. Presumably some other code was failing to clip to the root window's storage correctly.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.