[cairo] [PATCH 1/3] Enlarge fallback surface

[LRN]

On 17.04.2014 13:44, Uli Schlachter wrote:
> On 17.04.2014 11:06, LRN wrote:
>> Enlarges the fallback surface created of the amount
>> necessary not to write past the end of the DIB. It assumes that
>> Clip applied to an HDC are clamped to (0,0,width,height) of the HDC.
>>
>> Fixes bug 53121
> 
> How hard would it be to write a new test case for the test suite that catches
> this bug? This would mean it would need to be a test not using any
> (cairo-)win32-specific APIs.

No idea, since i've never tried to write a testcase for cairo testsuite.
That said, bug 53121 contains a minimal program that reproduces this crash,
maybe that could be used somehow?

> 
>> Signed-off-by: Massimo Valentini <mvalentini at src.gnome.org>
>>
>> ---
>>  src/win32/cairo-win32-display-surface.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/win32/cairo-win32-display-surface.c b/src/win32/cairo-win32-display-surface.c
>> index 5ecdbee..965f2c4 100644
>> --- a/src/win32/cairo-win32-display-surface.c
>> +++ b/src/win32/cairo-win32-display-surface.c
>> @@ -455,17 +455,17 @@ _cairo_win32_display_surface_map_to_image (void                    *abstract_sur
>>  	surface->fallback =
>>  	    _cairo_win32_display_surface_create_for_dc (surface->win32.dc,
>>  							surface->win32.format,
>> -							surface->win32.extents.width,
>> -							surface->win32.extents.height);
>> +							surface->win32.extents.x + surface->win32.extents.width,
>> +							surface->win32.extents.y + surface->win32.extents.height);
> 
> Can you explain (in the commit message) *why* this is necessary?

No, not really, since i am not the author of the patch. You may be able to get
some info from [1] and bug 53121.

> 
> This function is asked to create a surface covering the extents "extents".
> Apparently some caller is mis-using the returned surface and uses it outside of
> these extents. That sounds like a bug elsewhere.

That is certainly a possibility. Problem is, no one was able to find *where*
that "elsewhere" is.

In my case this bug surfaced (no pun intended) because a switch to RGBA pixel
format forced cairo to use fallback blitting functions instead of native W32
ones, which seem to be able to correctly restrain themselves and not write
outside of the surface memory region (unlike fallback functions, which just
do what the caller asks them to).
So my guess is that "elsewhere" is somewhere inside cairo-w32 backend.

> 
> You are fixing this by just handling back a bigger surface which means we get
> more memory pressure and lots of these memory is actually unused because it
> contains parts of an image which will just be discarded later on.

That looks like the case, yes. Better than crashing though, don't you think?


[1] http://lists.cairographics.org/archives/cairo/2013-September/024626.html