Fixed blit mapping problem when surfaces are freed and then newly allocated at the same address.
authorSam Lantinga <slouken@libsdl.org>
Mon, 16 Jan 2012 19:46:40 -0500
changeset 6222 84b67c1312c6
parent 6221 e54f601799eb
child 6223 af89a3a770e7
Fixed blit mapping problem when surfaces are freed and then newly allocated at the same address. Tim Angus to SDL 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.
src/video/SDL_pixels.c
--- a/src/video/SDL_pixels.c	Mon Jan 16 19:21:07 2012 -0500
+++ b/src/video/SDL_pixels.c	Mon Jan 16 19:46:40 2012 -0500
@@ -968,6 +968,12 @@
     if (!map) {
         return;
     }
+    if (map->dst) {
+        /* Release our reference to the surface - see the note below */
+        if (--map->dst->refcount <= 0) {
+            SDL_FreeSurface(map->dst);
+        }
+    }
     map->dst = NULL;
     map->src_palette_version = 0;
     map->dst_palette_version = 0;
@@ -1036,6 +1042,17 @@
 
     map->dst = dst;
 
+    if (map->dst) {
+        /* Keep a reference to this surface so it doesn't get deleted
+           while we're still pointing at it.
+
+           A better method would be for the destination surface to keep
+           track of surfaces that are mapped to it and automatically 
+           invalidate them when it is freed, but this will do for now.
+        */
+        ++map->dst->refcount;
+    }
+
     if (dstfmt->palette) {
         map->dst_palette_version = dstfmt->palette->version;
     } else {