Potentially nasty bug in surface blitting code

void function( SDL_Surface* surface )
{
SDL_Surface* anotherSurface =
SDL_ConvertSurfaceFormat( surface, … );

// surface->map->dst is now equal to anotherSurface

// Do some stuff with anotherSurface

SDL_FreeSurface( anotherSurface );

// anotherSurface is now a dead pointer,
// but surface->map->dst still points to it
}

int main( )
{
SDL_Surface* surface = CreateAValidSurface( );

function( surface );
}

At this point blit something from surface. SDL_LowerBlit is called,
which checks surface->map->dst against the blit destination. If the
pointers happen to match (not that unlikely), the map is decided to be
valid and bad things happen.

It seems to me like the whole idea of caching the blit mapping is
fundamentally flawed in that the source surface has no knowledge of the
lifetime of the destination surface. I’m working from an SDL2 snapshot
that’s a month or two old so this might have been fixed, but I couldn’t
see anything obvious in the commit log. I haven’t looked at 1.2.

Of course I might also be making some fundamental API usage mistake, in
which case apply the cluebat to my skull…

Does refcount affect this in any way?

Jonny DOn Thu, Jan 12, 2012 at 1:00 PM, Tim Angus wrote:

void function( SDL_Surface* surface )
{
SDL_Surface* anotherSurface =
SDL_ConvertSurfaceFormat( surface, … );

// surface->map->dst is now equal to anotherSurface

// Do some stuff with anotherSurface

SDL_FreeSurface( anotherSurface );

// anotherSurface is now a dead pointer,
// but surface->map->dst still points to it
}

int main( )
{
SDL_Surface* surface = CreateAValidSurface( );

function( surface );
}

At this point blit something from surface. SDL_LowerBlit is called, which
checks surface->map->dst against the blit destination. If the pointers
happen to match (not that unlikely), the map is decided to be valid and bad
things happen.

It seems to me like the whole idea of caching the blit mapping is
fundamentally flawed in that the source surface has no knowledge of the
lifetime of the destination surface. I’m working from an SDL2 snapshot
that’s a month or two old so this might have been fixed, but I couldn’t see
anything obvious in the commit log. I haven’t looked at 1.2.

Of course I might also be making some fundamental API usage mistake, in
which case apply the cluebat to my skull…
_____________**
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/**listinfo.cgi/sdl-libsdl.orghttp://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org

That is perhaps the problem. It doesn’t look like the refcount is ever
incremented except for when the surface is created. It might be that
whenever a blit map is created the refcount should be incremented and
decremented when it’s invalidated.On 12/01/2012 18:13, Jonathan Dearborn wrote:

Does refcount affect this in any way?

Indeed this fixed it. If the attached patch looks right or at least not
horrifically wrong I’ll create a bug it.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed…
Name: sdl-fix-missing-strong-ref.diff
URL: http://lists.libsdl.org/pipermail/sdl-libsdl.org/attachments/20120116/a78a59f9/attachment.ascOn 12/01/2012 18:22, Tim Angus wrote:

On 12/01/2012 18:13, Jonathan Dearborn wrote:

Does refcount affect this in any way?

That is perhaps the problem. It doesn’t look like the refcount is ever
incremented except for when the surface is created. It might be that
whenever a blit map is created the refcount should be incremented and
decremented when it’s invalidated.

Thanks Tim, I added a similar change, but did it inline with a note to
highlight the hack.On Mon, Jan 16, 2012 at 7:44 AM, Tim Angus wrote:

On 12/01/2012 18:22, Tim Angus wrote:

On 12/01/2012 18:13, Jonathan Dearborn wrote:

Does refcount affect this in any way?

That is perhaps the problem. It doesn’t look like the refcount is ever
incremented except for when the surface is created. It might be that
whenever a blit map is created the refcount should be incremented and
decremented when it’s invalidated.

Indeed this fixed it. If the attached patch looks right or at least not
horrifically wrong I’ll create a bug it.


SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org

Cool, thanks.On 17/01/2012 00:47, Sam Lantinga wrote:

Thanks Tim, I added a similar change, but did it inline with a note to
highlight the hack.