This bug was originally filed as part of poppler, but it seems to be a cairo
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)
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.
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?
Uli, your patch fixes another issue, please push.
Author: Chris Wilson <email@example.com>
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 <firstname.lastname@example.org>
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...
Author: Uli Schlachter <email@example.com>
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
Signed-off-by: Uli Schlachter <firstname.lastname@example.org>
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.
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.
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:
Thanks. I've verified that his patch fixes the issue. Thanks again!