[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg-devel
Subject:    Re: [PATCH xserver v2 2/2] glamor: Avoid overflow between box32 and box16 box
From:       Olivier Fourdan <ofourdan () redhat ! com>
Date:       2017-07-26 8:41:34
Message-ID: 1798588908.29717653.1501058494025.JavaMail.zimbra () redhat ! com
[Download RAW message or body]

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@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
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel


[prev in list] [next in list] [prev in thread] [next in thread]