[PATCH xserver v2 2/2] glamor: Avoid overflow between box32 and box16 box

[Olivier Fourdan]

Hi Michel,

> > > glamor_compute_transform_clipped_regions() uses a temporary box32
> > > internally which is copied back to a box16 to init the regions16,
> > > thus causing a potential overflow.
> > > 
> > > If an overflow occurs, the given region is invalid and the pixmap
> > > init region will fail.
> > > 
> > > Simply check that the coordinates won't overflow when copying back to
> > > the box16, avoiding a crash later down the line in glamor.
> > > 
> > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=101894
> > > Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> > > ---
> > >  v2: Make sure we have (x1,y1) < (x2,y2) in case of overflow to avoid an
> > >      empty region.
> > 
> > An empty region actually seems more appropriate to me in that case.
> > Maybe just don't call RegionInitBoxes if short_box is empty?
> 
> Thing is, looking at pixman's definition of GOOD_RECT() and BAD_RECT() [1],
> an empty region is neither, so not being a "GOOD_RECT()" we follow the same
> code path in RegionInitBoxes and therefore would most likely cause the same
> problems as with a BAD_RECT() as before.
> 
> glamor_compute_transform_clipped_regions() would return a NULL region [3] and
> we would be in the same case as with a null region that causes further
> problems down the line.

So, you're right (of course), I looked further into this and an empty region *should* work, only problem is glamor_composite_clipped_region() will fail if the source is NULL (which is the case as COMPOSITE_REGION() will pass NULL if the regions is empty), so we either hit a null pointer dereference in glamor_composite_choose_shader() or an assert(0) in the COMPOSITE_REGION() macro.

But the fix for this is trivial, as the code is supposed to be able to pass an empty region, glamor_composite_clipped_region() should be nice enough to not call glamor_composite_with_shader() if the source is NULL and return OK in this case (as this should be OK according to the rest of the caller stack).

Will post a third patch that does this (tested with the image provided in bug 101894 with my overflow patch reverted to make sure we have an empty region).

Cheers,
Olivier