| Message ID | 20160802173407.22362-1-aplattner@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 92b3cd32066aa5befa67a408cc079cd2ce4c077e |
| Headers | show |
| Series |
"xace: Fix XaceCensorImage to actually censor the right part of the image"
( rev:
1
)
in
X.org (DEPRECATED - USE GITLAB) |
diff --git a/Xext/xace.c b/Xext/xace.c index 91c74d591267..a3a83a20c08a 100644 --- a/Xext/xace.c +++ b/Xext/xace.c @@ -245,6 +245,7 @@ XaceCensorImage(ClientPtr client, /* censorRegion = imageRegion - visibleRegion */ RegionSubtract(&censorRegion, &imageRegion, pVisibleRegion); + RegionTranslate(&censorRegion, -x, -y); nRects = RegionNumRects(&censorRegion); if (nRects > 0) { /* we have something to censor */ GCPtr pScratchGC = NULL; @@ -265,7 +266,7 @@ XaceCensorImage(ClientPtr client, } for (pBox = RegionRects(&censorRegion), i = 0; i < nRects; i++, pBox++) { pRects[i].x = pBox->x1; - pRects[i].y = pBox->y1 - imageBox.y1; + pRects[i].y = pBox->y1; pRects[i].width = pBox->x2 - pBox->x1; pRects[i].height = pBox->y2 - pBox->y1; }
On Tue, 2016-08-02 at 10:34 -0700, Aaron Plattner wrote: > The caller passes arguments into XaceCensorImage that are in window-relative > coordinates. However, the pBuf that it uses to construct a temporary pixmap has > its origin at (x, y) relative to the window in question. The code to convert the > censor region into boxes adjusts for the Y coordinate, but leaves the X > coordinate alone. The result is that if x is not zero, it censors the wrong part > of the image. > > Fix this by just translating censorRegion into pixmap-relative coordinates and > using the resulting boxes as-is. > > Reported-by: Fabien Lelaquais <Fabien.Lelaquais@roguewave.com> > Link: https://lists.x.org/archives/xorg/2016-August/058165.html > Signed-off-by: Aaron Plattner <aplattner@nvidia.com> Nice find. remote: I: patch #102574 updated using rev 92b3cd32066aa5befa67a408cc079cd2ce4c077e. remote: I: 1 patch(es) updated to state Accepted. To ssh://git.freedesktop.org/git/xorg/xserver 4d58611..92b3cd3 master -> master - ajax
On 16/08/16 02:12 AM, Adam Jackson wrote: > On Tue, 2016-08-02 at 10:34 -0700, Aaron Plattner wrote: >> The caller passes arguments into XaceCensorImage that are in window-relative >> coordinates. However, the pBuf that it uses to construct a temporary pixmap has >> its origin at (x, y) relative to the window in question. The code to convert the >> censor region into boxes adjusts for the Y coordinate, but leaves the X >> coordinate alone. The result is that if x is not zero, it censors the wrong part >> of the image. >> >> Fix this by just translating censorRegion into pixmap-relative coordinates and >> using the resulting boxes as-is. >> >> Reported-by: Fabien Lelaquais <Fabien.Lelaquais@roguewave.com> >> Link: https://lists.x.org/archives/xorg/2016-August/058165.html >> Signed-off-by: Aaron Plattner <aplattner@nvidia.com> > > Nice find. > > remote: I: patch #102574 updated using rev 92b3cd32066aa5befa67a408cc079cd2ce4c077e. > remote: I: 1 patch(es) updated to state Accepted. > To ssh://git.freedesktop.org/git/xorg/xserver > 4d58611..92b3cd3 master -> master Unfortunately, this broke two XTS tests: xts5@xlib9@xgetimage@7 xts5@xlib9@xgetsubimage@7
On Thu, 2016-08-18 at 11:09 +0900, Michel Dänzer wrote: > Unfortunately, this broke two XTS tests: > > xts5@xlib9@xgetimage@7 > xts5@xlib9@xgetsubimage@7 Low impact, fortunately, but still unpleasant. The test in question is: 520|0 7 00020031 1 2|Assertion XGetImage-7.(A) 520|0 7 00020031 1 3|When the specified rectangle includes the window border, 520|0 7 00020031 1 4|then the contents of the window border are obtained in the 520|0 7 00020031 1 5|XImage structure returned by a call to XGetImage. I think there are two issues here. One is pVisibleRegion (the region we don't censor) is the intersection of borderClip (the exterior dimensions of the window including the border, clipped by siblings) and winSize (the inside-the-border region of the window). Clipping by winSize means we'll censor the window border. I think what's actually wanted there is borderClip also clipped by children [1]; we don't have a function handy to compute that, but it's straightforward enough. The other issue is we censor the image unconditionally if the server was built with support for any security extensions, regardless of whether the requesting client is trusted (for XC-SECURITY) or in a different security context than the window (for XACE). Patches forthcoming. [1] - Well kinda. You want to clip away children whose contents you aren't authorized to see, which isn't quite the same. - ajax
Aargh, stupid borders. I always forget about them. I guess this is why we have regression tests. On 08/18/2016 09:11 AM, Adam Jackson wrote: > On Thu, 2016-08-18 at 11:09 +0900, Michel Dänzer wrote: > >> Unfortunately, this broke two XTS tests: >> >> xts5@xlib9@xgetimage@7 >> xts5@xlib9@xgetsubimage@7 Thanks for catching this. > Low impact, fortunately, but still unpleasant. The test in question is: > > 520|0 7 00020031 1 2|Assertion XGetImage-7.(A) > 520|0 7 00020031 1 3|When the specified rectangle includes the window border, > 520|0 7 00020031 1 4|then the contents of the window border are obtained in the > 520|0 7 00020031 1 5|XImage structure returned by a call to XGetImage. > > I think there are two issues here. One is pVisibleRegion (the region we > don't censor) is the intersection of borderClip (the exterior > dimensions of the window including the border, clipped by siblings) and > winSize (the inside-the-border region of the window). Clipping by > winSize means we'll censor the window border. I think what's actually > wanted there is borderClip also clipped by children [1]; we don't have > a function handy to compute that, but it's straightforward enough. > > The other issue is we censor the image unconditionally if the server > was built with support for any security extensions, regardless of > whether the requesting client is trusted (for XC-SECURITY) or in a > different security context than the window (for XACE). > > Patches forthcoming. And thanks Adam for fixing it. > [1] - Well kinda. You want to clip away children whose contents you > aren't authorized to see, which isn't quite the same. > > - ajax >
Aaron Plattner <aplattner@nvidia.com> writes: > Aargh, stupid borders. I always forget about them. I guess this is why > we have regression tests. You just don't recall the whole original fight about borders. Left in X11 solely for X10 compatibility. Aren't you glad we have them?
The caller passes arguments into XaceCensorImage that are in window-relative coordinates. However, the pBuf that it uses to construct a temporary pixmap has its origin at (x, y) relative to the window in question. The code to convert the censor region into boxes adjusts for the Y coordinate, but leaves the X coordinate alone. The result is that if x is not zero, it censors the wrong part of the image. Fix this by just translating censorRegion into pixmap-relative coordinates and using the resulting boxes as-is. Reported-by: Fabien Lelaquais <Fabien.Lelaquais@roguewave.com> Link: https://lists.x.org/archives/xorg/2016-August/058165.html Signed-off-by: Aaron Plattner <aplattner@nvidia.com> --- Xext/xace.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)