Bug 75819

Summary: _cairo_clip_set_all_clipped returns const value as non-const, caller modifies it -> crash on some platforms
Product: cairo Reporter: Andrew de los Reyes <andrew-freedesktop>
Component: generalAssignee: Uli Schlachter <psychon>
Status: RESOLVED FIXED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: normal    
Priority: medium CC: andrew-freedesktop, mitar.freedesktop
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Make sure _cairo_clip_intersect_rectangle_box() doesn't get with the all-clipped clip

Description Andrew de los Reyes 2014-03-06 03:32:46 UTC
This bug was originally filed as part of poppler, but it seems to be a cairo 
issue: https://bugs.freedesktop.org/show_bug.cgi?id=74661

Here are the relevant code snippets:

>>>> cairo-clip.c: (on some platforms, this will be allocated to a read-only page of memory)

const cairo_clip_t __cairo_clip_all;


>>>> cairo-clip-inline.h: (returning __cairo_clip_all as non-const! uh oh...)

static inline cairo_clip_t *                                                                                                                                                                                  
_cairo_clip_set_all_clipped (cairo_clip_t *clip)                                                                                                                                                              
{                                                                                                                                                                                                             
    _cairo_clip_destroy (clip);                                                                                                                                                                               
    return (cairo_clip_t *) &__cairo_clip_all;                                                                                                                                                                
}                                                                                                                                                                                                             


>>>> cairo-clip-boxes.c: (mutate __cairo_clip_all -> CRASH!)

static cairo_clip_t *                                                                                                                                                                                         
_cairo_clip_intersect_rectangle_box (cairo_clip_t *clip,                                                                                                                                                      
                                     const cairo_rectangle_int_t *r,                                                                                                                                          
                                     const cairo_box_t *box)                                                                                                                                                  
{
...
                clip = _cairo_clip_set_all_clipped (clip);                                                                                                                                                    
...
            clip->is_region = _cairo_box_is_pixel_aligned (box);
            ^^^^^^^^^^^^^^^ bad write
...
}

I'm not a cairo expert by any stretch of the imagination, so I don't know the solution here. My gut reaction is to make the memory non-const so that it doesn't crash; that should at least work as well as systems where this doesn't crash. Still, it seems like someone who knows this code could see if there's a better solution.

Thanks,
-andrew
Comment 1 Uli Schlachter 2014-03-06 08:03:42 UTC
Created attachment 95210 [details] [review]
Make sure _cairo_clip_intersect_rectangle_box() doesn't get with the all-clipped clip

I wonder what Mac OS is doing differently than everyone else...

According to the stacktrace on the poppler bug report, the __cairo_clip_all constant is in the .text section. Weird.

Since we have no test case: Does the attached patch help?
Comment 2 Chris Wilson 2014-03-06 08:38:13 UTC
Uli, your patch fixes another issue, please push.

commit 3b261bea7d8e8094ff3899aefab6bbc8628a3585
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 6 08:34:10 2014 +0000

    clip: Do not modify the special all-clipped cairo_clip_t
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75819
Comment 3 Uli Schlachter 2014-03-06 08:55:43 UTC
Chris, done. For something completely different (which really doesn't belong into this comment): Could you take a look at _cairo_clip_reduce_to_boxes()? That function has an unconditional "return clip" with wrong indentation as its first executed statement. That doesn't seem right...

commit ed175b2a2bebb6def85133257bc11a875d13b0dd
Author: Uli Schlachter <psychon@znc.in>
Date:   Thu Mar 6 09:45:08 2014 +0100

    clip: Fix handling of special all-clipped cairo_clip_t
    
    _cairo_clip_intersect_box() wasn't checking if it was called with the special,
    read-only all-clipped clip and thus could have ended up writing to read-only
    memory.
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=75819
    Signed-off-by: Uli Schlachter <psychon@znc.in>
Comment 4 Chris Wilson 2014-03-06 09:48:06 UTC
I vaguely remember I never convinced myself that the path walking would be a win - or rather would more often be a loss than a win. So I left the code in just in case it turned out to be a good idea.
Comment 5 Andrew de los Reyes 2014-03-06 17:42:12 UTC
Hey, thanks for the quick response, but I verified that patch unfortunately doesn't fix the issue.

Have a closer look at the code snippets I posted when I first created this issue. Specifically, the body of _cairo_clip_intersect_rectangle_box . Note that this function is called with a perfectly mutable clip, but it then calls _cairo_clip_set_all_clipped which returns the immutable clip, which is then mutated.
Comment 6 Uli Schlachter 2014-03-06 17:50:36 UTC
You are right that my patch doesn't fix this issue. However, while I didn't figure out this bug, Chris did (and it was him who closed this). His patch is already in git. It can be found e.g. here:

http://cgit.freedesktop.org/cairo/commit/?id=3b261bea7d8e8094ff3899aefab6bbc8628a3585
Comment 7 Andrew de los Reyes 2014-03-06 18:07:35 UTC
Thanks. I've verified that his patch fixes the issue. Thanks again!

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.