Bug 77931 - NULL pointer dereference : _clip_and_composite_boxes() tries to destroy __cairo_clip_all's path
Summary: NULL pointer dereference : _clip_and_composite_boxes() tries to destroy __cai...
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: xcb backend (show other bugs)
Version: 1.12.16
Hardware: Other All
: medium normal
Assignee: Uli Schlachter
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
: 78181 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-04-25 14:40 UTC by Alexandre Rostovtsev
Modified: 2014-05-06 18:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch for cairo-xcb-surface-render (1.19 KB, patch)
2014-04-25 14:44 UTC, Alexandre Rostovtsev
Details | Splinter Review

Description Alexandre Rostovtsev 2014-04-25 14:40:25 UTC
(As reported downstream at https://bugs.gentoo.org/show_bug.cgi?id=507478)

Gentoo users who tried linking firefox-28 with vanilla cairo-1.12.16 with the xcb backend enabled (instead of using the patched cairo that's bundled with firefox) have reported NULL pointer dereferences in _cairo_clip_path_destroy().

The crash is caused by _clip_and_composite_boxes() failing to check whether the clip path being freed belongs to the constant __cairo_clip_all.
Comment 1 Alexandre Rostovtsev 2014-04-25 14:44:47 UTC
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.
Comment 2 Uli Schlachter 2014-04-26 22:29:03 UTC
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)
Comment 3 Alexandre Rostovtsev 2014-04-27 18:00:03 UTC
(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.
Comment 4 Alexandre Rostovtsev 2014-04-27 18:02:25 UTC
(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
}
Comment 5 Uli Schlachter 2014-05-02 09:48:05 UTC
*** Bug 78181 has been marked as a duplicate of this bug. ***
Comment 6 Uli Schlachter 2014-05-02 09:50:48 UTC
Another proposed patch can be found here:

http://lists.cairographics.org/archives/cairo/2014-May/025197.html
Comment 7 Alexandre Rostovtsev 2014-05-02 14:29:20 UTC
(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.
Comment 8 Uli Schlachter 2014-05-06 18:29:31 UTC
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!)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.