From befb5f7fb5b7f7d046e0a0b931b1fbfbb31ce06e Mon Sep 17 00:00:00 2001 From: Gabriel Ravier Date: Mon, 6 Jan 2020 10:43:28 +0100 Subject: [PATCH 1/3] ScaleAndUploadSurface now doesn't take ownership of the surface it is passed and frees it. This is to correct multiple occurences of use-after-free occuring from use of the passed surface after a call to ScaleAndUploadSurface using it --- src/Draw.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Draw.cpp b/src/Draw.cpp index 23932602..3a689262 100644 --- a/src/Draw.cpp +++ b/src/Draw.cpp @@ -160,8 +160,6 @@ static BOOL ScaleAndUploadSurface(SDL_Surface *surface, SurfaceID surf_no) { SDL_Surface *converted_surface = SDL_ConvertSurfaceFormat(surface, SDL_PIXELFORMAT_RGB24, 0); - SDL_FreeSurface(surface); - if (converted_surface == NULL) return FALSE; @@ -243,6 +241,7 @@ BOOL MakeSurface_Resource(const char *name, SurfaceID surf_no) if (!ScaleAndUploadSurface(surface, surf_no)) { Backend_FreeSurface(surf[surf_no]); + SDL_FreeSurface(surface); return FALSE; } @@ -251,6 +250,7 @@ BOOL MakeSurface_Resource(const char *name, SurfaceID surf_no) surface_metadata[surf_no].height = surface->h; surface_metadata[surf_no].bSystem = FALSE; strcpy(surface_metadata[surf_no].name, name); + SDL_FreeSurface(surface); return TRUE; } @@ -302,6 +302,7 @@ BOOL MakeSurface_File(const char *name, SurfaceID surf_no) if (!ScaleAndUploadSurface(surface, surf_no)) { Backend_FreeSurface(surf[surf_no]); + SDL_FreeSurface(surface); return FALSE; } @@ -310,6 +311,7 @@ BOOL MakeSurface_File(const char *name, SurfaceID surf_no) surface_metadata[surf_no].height = surface->h; surface_metadata[surf_no].bSystem = FALSE; strcpy(surface_metadata[surf_no].name, name); + SDL_FreeSurface(surface); return TRUE; } @@ -327,7 +329,12 @@ BOOL ReloadBitmap_Resource(const char *name, SurfaceID surf_no) SDL_Surface *surface = SDL_LoadBMP_RW(fp, 1); if (!ScaleAndUploadSurface(surface, surf_no)) + { + SDL_FreeSurface(surface); return FALSE; + } + + SDL_FreeSurface(surface); surface_metadata[surf_no].type = SURFACE_SOURCE_RESOURCE; strcpy(surface_metadata[surf_no].name, name); @@ -366,8 +373,12 @@ BOOL ReloadBitmap_File(const char *name, SurfaceID surf_no) } if (!ScaleAndUploadSurface(surface, surf_no)) + { + SDL_FreeSurface(surface); return FALSE; + } + SDL_FreeSurface(surface); surface_metadata[surf_no].type = SURFACE_SOURCE_FILE; strcpy(surface_metadata[surf_no].name, name); From 752b4cee3f9dd968e9129b185fca90bb76b9dede Mon Sep 17 00:00:00 2001 From: Gabriel Ravier Date: Mon, 6 Jan 2020 10:53:43 +0100 Subject: [PATCH 2/3] Correct bug in which Backend_LockSurface would not initialize surface->pixels, leaving a bug in which Backend_UnlockSurface used uninitialized values in certain scenarios (such as in ScaleAndUploadSurface) Signed-off-by: Gabriel Ravier --- src/Backends/Rendering/SDLTexture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Backends/Rendering/SDLTexture.cpp b/src/Backends/Rendering/SDLTexture.cpp index 341e70b8..e2acb50b 100644 --- a/src/Backends/Rendering/SDLTexture.cpp +++ b/src/Backends/Rendering/SDLTexture.cpp @@ -154,7 +154,7 @@ unsigned char* Backend_LockSurface(Backend_Surface *surface, unsigned int *pitch *pitch = surface->width * 3; - surface->pixels = (unsigned char*)malloc(surface->width * surface->height * 3); + surface->pixels = (unsigned char*)calloc(surface->width * surface->height * 3, 1); // Make sure these are initialized return surface->pixels; } From 2911bfda5c6051a768dd012e98aa6eae7f82b7f0 Mon Sep 17 00:00:00 2001 From: Gabriel Ravier Date: Mon, 6 Jan 2020 12:02:32 +0100 Subject: [PATCH 3/3] Removed memory leak in Backend_LoadGlyph Signed-off-by: Gabriel Ravier --- src/Backends/Rendering/SDLTexture.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Backends/Rendering/SDLTexture.cpp b/src/Backends/Rendering/SDLTexture.cpp index e2acb50b..05ae0eca 100644 --- a/src/Backends/Rendering/SDLTexture.cpp +++ b/src/Backends/Rendering/SDLTexture.cpp @@ -303,6 +303,7 @@ Backend_Glyph* Backend_LoadGlyph(const unsigned char *pixels, unsigned int width } SDL_UpdateTexture(glyph->texture, NULL, buffer, width * 4); + free(buffer); glyph->width = width; glyph->height = height;