[Pixman] [PATCH] _intersect_rect should return an empty region when intersecting with an empty rect

[Siarhei Siamashka]

On Wed, 13 Jan 2016 13:43:39 -0800
Bill McCloskey <wmccloskey at mozilla.com> wrote:

> When you call _intersect_rect and pass it an empty rectangle,
> it creates an invalid region from that rectangle. The only valid
> empty region is the one where the data pointer points to
> pixman_region_empty_data. The region created by _intersect_rect has
> a null data pointer, and so _not_empty returns true for it even if
> the extents are empty.
> 
> This patch uses code similar to what's in _union_rect to handle empty
> rectangles.
> ---
>  pixman/pixman-region.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
> index 59bc9c7..95911db 100644
> --- a/pixman/pixman-region.c
> +++ b/pixman/pixman-region.c
> @@ -1329,16 +1329,25 @@ PREFIX(_intersect_rect) (region_type_t *dest,
>      region_type_t region;
> 
>      region.data = NULL;
>      region.extents.x1 = x;
>      region.extents.y1 = y;
>      region.extents.x2 = x + width;
>      region.extents.y2 = y + height;
> 
> +    if (!GOOD_RECT (&region.extents))
> +    {
> +        if (BAD_RECT (&region.extents))
> +            _pixman_log_error (FUNC, "Invalid rectangle passed");
> +        FREE_DATA (dest);
> +        PREFIX (_init) (dest);
> +        return TRUE;
> +    }
> +
>      return PREFIX(_intersect) (dest, source, &region);
>  }
> 
>  /* Convenience function for performing union of region with a
>   * single rectangle
>   */
>   */
>  PIXMAN_EXPORT pixman_bool_t
>  PREFIX (_union_rect) (region_type_t *dest,

Thanks for the patch. This looks like a reasonable fix to me.

Just a minor nitpick. Currently the intersection of an empty region
and an invalid region is expected to be invalid [1]. But with your
patch, the intersection of an invalid region and an empty rectangle
is going to be empty. Which is a little bit inconsistent.

Also would it be too much to ask you to extend the test suite with a
test for this particular usage of this function? You can find some
information in the README file [2]. Basically, as the README file
instructs, please create two patches. The first one should extend the
test suite to demonstrate the bug. The second should fix the bug and
make the tests pass again. Please have a look at the existing region*
tests in the "test" directory.

And finally, since some time ago, pixman contributors are using
a "Signed-off-by:" tag in patches. Please add it to your commit
messages. With all of the above addressed, consider your patch

   Reviewed-by: Siarhei Siamashka <siarhei.siamashka at gmail.com>

[1] http://cgit.freedesktop.org/pixman/tree/pixman/pixman-region.c?id=pixman-0.33.6#n1173
[2] http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.6#n72

-- 
Best regards,
Siarhei Siamashka