[PATCH xserver] glamor: Fix temporary pixmap coordinate offsets

[Keith Packard]

Michel Dänzer <michel at daenzer.net> writes:

> From: Michel Dänzer <michel.daenzer at amd.com>
>
> Fixes crash with xli. No regressions with xterm, x11perf -copyplane or
> the xscreensaver phosphor hack.
>
> Bug: https://bugs.debian.org/857983
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  glamor/glamor_copy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
> index ff8f44ef1..ed96b2b1e 100644
> --- a/glamor/glamor_copy.c
> +++ b/glamor/glamor_copy.c
> @@ -230,8 +230,8 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>              goto bail;
>          }
>  
> -        src_pix->drawable.x = -dst->x;
> -        src_pix->drawable.y = -dst->y;
> +        src_pix->drawable.x = dst_xoff;
> +        src_pix->drawable.y = dst_yoff;

Ok, let's dig in and try to figure out why the old code was 'almost
right', and if the new code is 'always right.


dst_xoff/yoff came from glamor_get_drawable_deltas:

        dst_xoff = -pixmap->screen_x
        dst_yoff = -pixmap->screen_y

screen_x/screen_y are the location on the screen of the contents of the
pixmap when it's a Composite backing pixmap. Which is to say that the
pixel at 0,0 in the pixmap should be put onto the screen at
screen_x,screen_y when composited. When the destination is not a
composite backing pixmap, these coordinates are 0,0.

This value is useful when translating between screen-relative
coordinates and pixmap relative coordinates.

screen_x,screen_y differs from dst->x,dst->y when the destination window
has a border (which never happens these days), or when the destination
window is a subwindow (rare, but still does happen).

Now let's think about the coordinate space of the incoming values to
glamor_copy_cpu_fbo. The relevant parameters are:

        BoxPtr box;
        int nbox;
        int dx, dy;

Those come from miDoCopy which computes a screen-relative region
covering the destination of the copy operation. dx,dy are offsets from
the destination coordinates to the source coordinates such that you add
dx,dy to the box coordinates to get screen-relative source coordinates.

glamor_copy_cpu_fbo doesn't want to mess with dx,dy, instead it wants to
compute drawable.x,drawable.y values for the temporary pixmap which
makes things work out. I'm not sure that's entirely possible, but let's
carry on.

The first thing we need to do is expand the source plane into fg/bg
colors in the temporary pixmap:

    fbCopy1toN(src, &tmp_pix->drawable, gc, box, nbox, dx, dy,
               reverse, upsidedown, bitplane, closure);

This function gets some destination offsets, which end up being:

        dstXoff = src_pix->drawable.x
        dstYoff = src_pix->drawable.y

it then does a blt, computing the address of the scanline containing the
first pixel to copy (we'll look at Y as it's a bit easier to follow):

        tmp_pix->devPrivate.ptr + (pbox->y1 + dstYoff) * dstStride

Given that pbox is screen-relative, it's pretty clear that dstYoff needs
to adjust for the relationship between the temporary pixmap and the
screen. As the pixmap was allocated to match the composite backing
pixmap, and not the destination drawable itself, that offset should be
the offset of the composite pixmap on the screen or -dst_pixmap->screen_y. 

        dstYoff = src_pix->drawable.y = - dst_pixmap->screen_y

dst_pixmap->screen_y should only be used if we're actually
composited. Fortunately, dst_yoff is computed exactly that way, so I
think we have

        src_pix->drawable.x = dst_xoff;
        src_pix->drawable.y = dst_yoff;

I spent some time thinking about the possibility that the temporary
pixmap might have been allocated such that drawable.x,drawable.y weren't
zero. If that were true, then we'd have to add these new offsets instead
of just assigning them:

        src_pix->drawable.x += dst_xoff;
        src_pix->drawable.y += dst_yoff;

Given that it's an in-memory pixmap allocated with a direct call to
fbCreatePixmap, this "can't ever happen", and so I think it's better to
use the simpler version above.

Now, let's see how that fares with the second step of the process, which
takes bits from this temporary pixmap and uploads them into the FBO for
the destination:

        glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff, src_yoff,
                            dst_xoff, dst_yoff, (uint8_t *) src_bits,
                            src_stride * sizeof(FbBits));

Note that src_xoff, src_yoff are computed from the temporary pixmap, not
the actual source. This caught me for a moment, so of course

        src_xoff = src_pix->drawable.x
        src_yoff = src_pix->drawable.y

And, we already know what those values are from above:

        src_xoff = dst_xoff;
        src_yoff = dst_yoff;

How convenient; we've got a temporary pixmap which now has bits exactly
aligned with the target, so the two offsets are equal, and so we'll
fetch bits from locations matching where we're storing them.

Now, as to why the existing code was *almost* correct, you'll see from
the above that the only time when dst_pixmap->screen_x != dst->x is when
dst has a border or is a subwindow, so for simple tests, the values
always match and so it works. xli creates a large subwindow the size of
the full image and places that within the parent offset by the scrolled
amount. Scroll far enough, and the difference between dst_xoff and
-dst->x becomes large enough to eventually crash the server.

Reviewed-by: Keith Packard <keithp at keithp.com>

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170612/bbaf7865/attachment.sig>