Bug 77931

Created attachment 97959 [details] [review]
patch for cairo-xcb-surface-render

This patch for cairo-xcb-surface-render's _clip_and_composite_boxes(), in addition to two other all-clipped clip handling patches that are already in cairo git (3b261bea and ed175b2a), fixes the crash for our users.

Sadly no clear test case for this.

Since drawing with an all-clipped clip doesn't do anything at all, something should notice this and return success early on.

Instead of your patch, could you try adding the following at the beginning of _clip_and_composite_boxes()? Thanks (Hm, and I'd be curious how exactly this can happen at all, the higher levels should check for all-clipped earlier, I thought):

if (_cairo_clip_is_all_clipped (clip))
  return CAIRO_STATUS_SUCCESS;

(Sorry, my main work computer broke and I am having some problems due to this)

(In reply to comment #2)
> Instead of your patch, could you try adding the following at the beginning
> of _clip_and_composite_boxes()? Thanks (Hm, and I'd be curious how exactly
> this can happen at all, the higher levels should check for all-clipped
> earlier, I thought):
> 
> if (_cairo_clip_is_all_clipped (clip))
>   return CAIRO_STATUS_SUCCESS;

I cannot see how that could work.

Look at the abbreviated logic of _clip_and_composite_boxes() :

if ( extents->clip->path != NULL ) {
    cairo_clip_t *clip;
    clip = _cairo_clip_copy (extents->clip);
    clip = _cairo_clip_intersect_boxes (clip, boxes);
    clip = _cairo_clip_intersect_boxes (clip, boxes); // this crashes due to NULL dereference
}

Since we know that extents->clip->path is not NULL, we are guaranteed that the initial value of clip is *not* the all-clipped path. In other words, the all-clipped path is coming from result of _cairo_clip_intersect_boxes(), not from the parameters to _clip_and_composite_boxes(). So checking for all-clipped at the beginning of _clip_and_composite_boxes() won't help.

(In reply to comment #3)
> if ( extents->clip->path != NULL ) {
>     cairo_clip_t *clip;
>     clip = _cairo_clip_copy (extents->clip);
>     clip = _cairo_clip_intersect_boxes (clip, boxes);
>     clip = _cairo_clip_intersect_boxes (clip, boxes); // this crashes due to  NULL dereference
> }

Typo, meant to say:

if ( extents->clip->path != NULL ) {
    cairo_clip_t *clip;
    clip = _cairo_clip_copy (extents->clip);
    clip = _cairo_clip_intersect_boxes (clip, boxes);
    clip = _cairo_clip_path_destroy (clip->path); // this crashes due to NULL dereference
}

*** Bug 78181 has been marked as a duplicate of this bug. ***

Another proposed patch can be found here:

http://lists.cairographics.org/archives/cairo/2014-May/025197.html

(In reply to comment #6)
> Another proposed patch can be found here:
> 
> http://lists.cairographics.org/archives/cairo/2014-May/025197.html

That should also work.

Even though the commit doesn't say so:

Fixed by

commit 18b3cce2f5812c357e4b6310e72d72dd9ec92ed4
Author: Bryce Harrington <b.harrington@samsung.com>
Date:   Tue May 6 10:18:19 2014 -0700

    Fix segfault in firefox when scrolling on certain pages
    
    Bug discovered by thorsten <fly_a320@gmx.de>
    
    Patch from Chris Wilson <chris@chris-wilson.co.uk>

(Thanks, Bryce!)