From 8830b466d0bfd2429879b7e2189119c0898ceda7 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Wed, 8 Oct 2025 10:00:21 -0700
Subject: [PATCH] Improve handling of surfaces with NULL pixels
Fixes https://github.com/libsdl-org/SDL/issues/14059
---
src/video/SDL_fillrect.c | 11 ++-
src/video/SDL_surface.c | 136 +++++++++++++++++++-----------
test/testautomation_surface.c | 150 +++++++++++++++++++++++++++++++++-
3 files changed, 238 insertions(+), 59 deletions(-)
diff --git a/src/video/SDL_fillrect.c b/src/video/SDL_fillrect.c
index a32c3c6c646c6..5be1eec52784e 100644
--- a/src/video/SDL_fillrect.c
+++ b/src/video/SDL_fillrect.c
@@ -266,17 +266,16 @@ bool SDL_FillSurfaceRects(SDL_Surface *dst, const SDL_Rect *rects, int count, Ui
return SDL_InvalidParamError("SDL_FillSurfaceRects(): dst");
}
- // Perform software fill
- CHECK_PARAM(!dst->pixels) {
- return SDL_SetError("SDL_FillSurfaceRects(): You must lock the surface");
- }
-
CHECK_PARAM(!rects) {
return SDL_InvalidParamError("SDL_FillSurfaceRects(): rects");
}
+ if (!dst->pixels && SDL_MUSTLOCK(dst)) {
+ return SDL_SetError("SDL_FillSurfaceRects(): You must lock the surface");
+ }
+
// Nothing to do
- if (dst->w == 0 || dst->h == 0) {
+ if (dst->w == 0 || dst->h == 0 || !dst->pixels) {
return true;
}
diff --git a/src/video/SDL_surface.c b/src/video/SDL_surface.c
index f2cffae99f4f1..82a24c60ebadd 100644
--- a/src/video/SDL_surface.c
+++ b/src/video/SDL_surface.c
@@ -598,7 +598,7 @@ bool SDL_SetSurfaceRLE(SDL_Surface *surface, bool enabled)
{
int flags;
- CHECK_PARAM(!SDL_SurfaceValid(surface)) {
+ CHECK_PARAM(!SDL_SurfaceValid(surface) || SDL_ISPIXELFORMAT_FOURCC(surface->format)) {
return SDL_InvalidParamError("surface");
}
@@ -1017,10 +1017,10 @@ bool SDL_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst
SDL_Rect r_src, r_dst;
// Make sure the surfaces aren't locked
- CHECK_PARAM(!SDL_SurfaceValid(src)) {
+ CHECK_PARAM(!SDL_SurfaceValid(src) || (!src->pixels && !SDL_MUSTLOCK(src))) {
return SDL_InvalidParamError("src");
}
- CHECK_PARAM(!SDL_SurfaceValid(dst)) {
+ CHECK_PARAM(!SDL_SurfaceValid(dst) || (!dst->pixels && !SDL_MUSTLOCK(dst))) {
return SDL_InvalidParamError("dst");
}
CHECK_PARAM((src->flags & SDL_SURFACE_LOCKED) || (dst->flags & SDL_SURFACE_LOCKED)) {
@@ -1094,12 +1094,6 @@ bool SDL_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst
static bool SDL_BlitSurfaceClippedScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst, const SDL_Rect *dstrect, SDL_ScaleMode scaleMode)
{
// We need to scale first, then blit into dst because we're clipping in the destination surface pixel coordinates
- if (SDL_MUSTLOCK(src)) {
- if (!SDL_LockSurface(src)) {
- return false;
- }
- }
-
bool result;
int saved_w = src->w;
int saved_h = src->h;
@@ -1117,22 +1111,19 @@ static bool SDL_BlitSurfaceClippedScaled(SDL_Surface *src, const SDL_Rect *srcre
src->w = saved_w;
src->h = saved_h;
src->pixels = saved_pixels;
-
- if (SDL_MUSTLOCK(src)) {
- SDL_UnlockSurface(src);
- }
return result;
}
bool SDL_BlitSurfaceScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst, const SDL_Rect *dstrect, SDL_ScaleMode scaleMode)
{
SDL_Rect r_src, r_dst;
+ bool result;
// Make sure the surfaces aren't locked
- CHECK_PARAM(!SDL_SurfaceValid(src) || !src->pixels) {
+ CHECK_PARAM(!SDL_SurfaceValid(src) || (!src->pixels && !SDL_MUSTLOCK(src))) {
return SDL_InvalidParamError("src");
}
- CHECK_PARAM(!SDL_SurfaceValid(dst) || !dst->pixels) {
+ CHECK_PARAM(!SDL_SurfaceValid(dst) || (!dst->pixels && !SDL_MUSTLOCK(dst))) {
return SDL_InvalidParamError("dst");
}
CHECK_PARAM((src->flags & SDL_SURFACE_LOCKED) || (dst->flags & SDL_SURFACE_LOCKED)) {
@@ -1214,7 +1205,29 @@ bool SDL_BlitSurfaceScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surfac
return SDL_BlitSurfaceClippedScaled(src, &r_src, dst, &r_dst, scaleMode);
}
- return SDL_BlitSurfaceUncheckedScaled(src, &r_src, dst, &r_dst, scaleMode);
+ if (SDL_MUSTLOCK(src)) {
+ if (!SDL_LockSurface(src)) {
+ return false;
+ }
+ }
+ if (SDL_MUSTLOCK(dst)) {
+ if (!SDL_LockSurface(dst)) {
+ if (SDL_MUSTLOCK(src)) {
+ SDL_UnlockSurface(src);
+ }
+ return false;
+ }
+ }
+
+ result = SDL_BlitSurfaceUncheckedScaled(src, &r_src, dst, &r_dst, scaleMode);
+
+ if (SDL_MUSTLOCK(src)) {
+ SDL_UnlockSurface(src);
+ }
+ if (SDL_MUSTLOCK(dst)) {
+ SDL_UnlockSurface(dst);
+ }
+ return result;
}
/**
@@ -1900,7 +1913,11 @@ SDL_Surface *SDL_ConvertSurfaceAndColorspace(SDL_Surface *surface, SDL_PixelForm
src_properties = surface->props;
// Create a new surface with the desired format
- convert = SDL_CreateSurface(surface->w, surface->h, format);
+ if (surface->pixels || SDL_MUSTLOCK(surface)) {
+ convert = SDL_CreateSurface(surface->w, surface->h, format);
+ } else {
+ convert = SDL_CreateSurfaceFrom(surface->w, surface->h, format, NULL, 0);
+ }
if (!convert) {
goto error;
}
@@ -1992,7 +2009,11 @@ SDL_Surface *SDL_ConvertSurfaceAndColorspace(SDL_Surface *surface, SDL_PixelForm
}
}
- result = SDL_BlitSurfaceUnchecked(surface, &bounds, convert, &bounds);
+ if (surface->pixels) {
+ result = SDL_BlitSurfaceUnchecked(surface, &bounds, convert, &bounds);
+ } else {
+ result = true;
+ }
// Restore colorkey alpha value
if (palette_ck_transform) {
@@ -2177,40 +2198,49 @@ SDL_Surface *SDL_ScaleSurface(SDL_Surface *surface, int width, int height, SDL_S
}
// Create a new surface with the desired size
- convert = SDL_CreateSurface(width, height, surface->format);
+ if (surface->pixels || SDL_MUSTLOCK(surface)) {
+ convert = SDL_CreateSurface(width, height, surface->format);
+ } else {
+ convert = SDL_CreateSurfaceFrom(width, height, surface->format, NULL, 0);
+ }
if (!convert) {
goto error;
}
SDL_SetSurfacePalette(convert, surface->palette);
SDL_SetSurfaceColorspace(convert, surface->colorspace);
+ SDL_SetSurfaceRLE(convert, SDL_SurfaceHasRLE(surface));
- // Save the original copy flags
- copy_flags = surface->map.info.flags;
- copy_color.r = surface->map.info.r;
- copy_color.g = surface->map.info.g;
- copy_color.b = surface->map.info.b;
- copy_color.a = surface->map.info.a;
- surface->map.info.r = 0xFF;
- surface->map.info.g = 0xFF;
- surface->map.info.b = 0xFF;
- surface->map.info.a = 0xFF;
- surface->map.info.flags = (copy_flags & (SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY));
- SDL_InvalidateMap(&surface->map);
-
- rc = SDL_BlitSurfaceScaled(surface, NULL, convert, NULL, scaleMode);
+ if (surface->pixels || SDL_MUSTLOCK(surface)) {
+ // Save the original copy flags
+ copy_flags = surface->map.info.flags;
+ copy_color.r = surface->map.info.r;
+ copy_color.g = surface->map.info.g;
+ copy_color.b = surface->map.info.b;
+ copy_color.a = surface->map.info.a;
+ surface->map.info.r = 0xFF;
+ surface->map.info.g = 0xFF;
+ surface->map.info.b = 0xFF;
+ surface->map.info.a = 0xFF;
+ surface->map.info.flags = (copy_flags & (SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY));
+ SDL_InvalidateMap(&surface->map);
- // Clean up the original surface, and update converted surface
- convert->map.info.r = copy_color.r;
- convert->map.info.g = copy_color.g;
- convert->map.info.b = copy_color.b;
- convert->map.info.a = copy_color.a;
- convert->map.info.flags = (copy_flags & ~(SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY));
- surface->map.info.r = copy_color.r;
- surface->map.info.g = copy_color.g;
- surface->map.info.b = copy_color.b;
- surface->map.info.a = copy_color.a;
- surface->map.info.flags = copy_flags;
- SDL_InvalidateMap(&surface->map);
+ rc = SDL_BlitSurfaceScaled(surface, NULL, convert, NULL, scaleMode);
+
+ // Clean up the original surface, and update converted surface
+ convert->map.info.r = copy_color.r;
+ convert->map.info.g = copy_color.g;
+ convert->map.info.b = copy_color.b;
+ convert->map.info.a = copy_color.a;
+ convert->map.info.flags = (copy_flags & ~(SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY));
+ surface->map.info.r = copy_color.r;
+ surface->map.info.g = copy_color.g;
+ surface->map.info.b = copy_color.b;
+ surface->map.info.a = copy_color.a;
+ surface->map.info.flags = copy_flags;
+ SDL_InvalidateMap(&surface->map);
+ } else {
+ rc = true;
+ }
// SDL_BlitSurfaceScaled failed, and so the conversion
if (!rc) {
@@ -2239,8 +2269,18 @@ SDL_Surface *SDL_ConvertSurface(SDL_Surface *surface, SDL_PixelFormat format)
SDL_Surface *SDL_DuplicatePixels(int width, int height, SDL_PixelFormat format, SDL_Colorspace colorspace, void *pixels, int pitch)
{
- SDL_Surface *surface = SDL_CreateSurface(width, height, format);
- if (surface) {
+ SDL_Surface *surface;
+ if (pixels) {
+ surface = SDL_CreateSurface(width, height, format);
+ } else {
+ surface = SDL_CreateSurfaceFrom(width, height, format, NULL, 0);
+ }
+ if (!surface) {
+ return NULL;
+ }
+ SDL_SetSurfaceColorspace(surface, colorspace);
+
+ if (surface->pixels) {
int length = width * SDL_BYTESPERPIXEL(format);
Uint8 *src = (Uint8 *)pixels;
Uint8 *dst = (Uint8 *)surface->pixels;
@@ -2250,8 +2290,6 @@ SDL_Surface *SDL_DuplicatePixels(int width, int height, SDL_PixelFormat format,
dst += surface->pitch;
src += pitch;
}
-
- SDL_SetSurfaceColorspace(surface, colorspace);
}
return surface;
}
diff --git a/test/testautomation_surface.c b/test/testautomation_surface.c
index c3aededf91802..13b979a12e585 100644
--- a/test/testautomation_surface.c
+++ b/test/testautomation_surface.c
@@ -707,6 +707,140 @@ static int SDLCALL surface_testBlitMultiple(void *arg)
return TEST_COMPLETED;
}
+/**
+ * Tests operations on surfaces with NULL pixels
+ */
+static int SDLCALL surface_testSurfaceNULLPixels(void *arg)
+{
+ SDL_Surface *a, *b, *face;
+ bool result;
+
+ face = SDLTest_ImageFace();
+ SDLTest_AssertCheck(face != NULL, "Verify face surface is not NULL");
+ if (face == NULL) {
+ return TEST_ABORTED;
+ }
+
+ /* Test blitting with NULL pixels */
+ a = SDL_CreateSurfaceFrom(face->w, face->h, SDL_PIXELFORMAT_ARGB8888, NULL, 0);
+ SDLTest_AssertCheck(a != NULL, "Verify result from SDL_CreateSurfaceFrom() with NULL pixels is not NULL");
+ result = SDL_BlitSurface(a, NULL, face, NULL);
+ SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurface() with src having NULL pixels is false");
+ result = SDL_BlitSurface(face, NULL, a, NULL);
+ SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurface() with dst having NULL pixels is false");
+
+ b = SDL_CreateSurfaceFrom(face->w * 2, face->h * 2, SDL_PIXELFORMAT_ARGB8888, NULL, 0);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_CreateSurfaceFrom() with NULL pixels is not NULL");
+ result = SDL_BlitSurfaceScaled(b, NULL, face, NULL, SDL_SCALEMODE_NEAREST);
+ SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurfaceScaled() with src having NULL pixels is false");
+ result = SDL_BlitSurfaceScaled(face, NULL, b, NULL, SDL_SCALEMODE_NEAREST);
+ SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurfaceScaled() with dst having NULL pixels is false");
+ SDL_DestroySurface(b);
+ b = NULL;
+
+ /* Test conversion with NULL pixels */
+ b = SDL_ConvertSurfaceAndColorspace(a, SDL_PIXELFORMAT_ABGR8888, NULL, SDL_COLORSPACE_UNKNOWN, 0);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ConvertSurfaceAndColorspace() with NULL pixels is not NULL");
+ SDL_DestroySurface(b);
+ b = NULL;
+
+ /* Test duplication with NULL pixels */
+ b = SDL_DuplicateSurface(a);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_DuplicateSurface() with NULL pixels is not NULL");
+ SDL_DestroySurface(b);
+ b = NULL;
+
+ /* Test scaling with NULL pixels */
+ b = SDL_ScaleSurface(a, a->w * 2, a->h * 2, SDL_SCALEMODE_NEAREST);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ScaleSurface() with NULL pixels is not NULL");
+ SDLTest_AssertCheck(b->pixels == NULL, "Verify pixels from SDL_ScaleSurface() is NULL");
+ SDL_DestroySurface(b);
+ b = NULL;
+
+ /* Test filling surface with NULL pixels */
+ result = SDL_FillSurfaceRect(a, NULL, 0);
+ SDLTest_AssertCheck(result, "Verify result from SDL_FillSurfaceRect() with dst having NULL pixels is true");
+
+ /* Clean up. */
+ SDL_DestroySurface(face);
+ SDL_DestroySurface(a);
+ SDL_DestroySurface(b);
+
+ return TEST_COMPLETED;
+}
+
+/**
+ * Tests operations on surfaces with RLE pixels
+ */
+static int SDLCALL surface_testSurfaceRLEPixels(void *arg)
+{
+ SDL_Surface *face, *a, *b, *tmp;
+ bool result;
+
+ face = SDLTest_ImageFace();
+ SDLTest_AssertCheck(face != NULL, "Verify face surface is not NULL");
+ if (face == NULL) {
+ return TEST_ABORTED;
+ }
+
+ /* Create a temporary surface to trigger RLE encoding during blit */
+ tmp = SDL_DuplicateSurface(face);
+ SDLTest_AssertCheck(tmp != NULL, "Verify result from SDL_DuplicateSurface() with RLE pixels is not NULL");
+
+ result = SDL_SetSurfaceRLE(face, true);
+ SDLTest_AssertCheck(result, "Verify result from SDL_SetSurfaceRLE() is true");
+
+ /* Test duplication with RLE pixels */
+ a = SDL_DuplicateSurface(face);
+ SDLTest_AssertCheck(a != NULL, "Verify result from SDL_DuplicateSurface() with RLE pixels is not NULL");
+ SDLTest_AssertCheck(SDL_SurfaceHasRLE(a), "Verify result from SDL_DuplicateSurface() with RLE pixels has RLE set");
+
+ /* Verify that blitting from an RLE surface does RLE encode it */
+ SDLTest_AssertCheck(!SDL_MUSTLOCK(a), "Verify initial RLE surface does not need to be locked");
+ SDLTest_AssertCheck(a->pixels != NULL, "Verify initial RLE surface has pixels available");
+ result = SDL_BlitSurface(a, NULL, tmp, NULL);
+ SDLTest_AssertCheck(result, "Verify result from SDL_BlitSurface() with RLE surface is true");
+ SDLTest_AssertCheck(SDL_MUSTLOCK(a), "Verify RLE surface after blit needs to be locked");
+ SDLTest_AssertCheck(a->pixels == NULL, "Verify RLE surface after blit does not have pixels available");
+
+ /* Test scaling with RLE pixels */
+ b = SDL_ScaleSurface(a, a->w * 2, a->h * 2, SDL_SCALEMODE_NEAREST);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ScaleSurface() is not NULL");
+ SDLTest_AssertCheck(SDL_SurfaceHasRLE(b), "Verify result from SDL_ScaleSurface() with RLE pixels has RLE set");
+
+ /* Test scaling blitting with RLE pixels */
+ result = SDL_BlitSurfaceScaled(a, NULL, b, NULL, SDL_SCALEMODE_NEAREST);
+ SDLTest_AssertCheck(result, "Verify result from SDL_BlitSurfaceScaled() with src having RLE pixels is true");
+ SDL_BlitSurface(a, NULL, tmp, NULL);
+ SDL_DestroySurface(b);
+ b = NULL;
+
+ /* Test conversion with RLE pixels */
+ b = SDL_ConvertSurfaceAndColorspace(a, SDL_PIXELFORMAT_ABGR8888, NULL, SDL_COLORSPACE_UNKNOWN, 0);
+ SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ConvertSurfaceAndColorspace() with RLE pixels is not NULL");
+ SDLTest_AssertCheck(SDL_SurfaceHasRLE(b), "Verify result from SDL_ConvertSurfaceAndColorspace() with RLE pixels has RLE set");
+ SDL_BlitSurface(a, NULL, tmp, NULL);
+ SDL_DestroySurface(b);
+ b = NULL;
+
+#if 0 /* This will currently fail, you must lock the surface first */
+ /* Test filling surface with RLE pixels */
+ result = SDL_FillSurfaceRect(a, NULL, 0);
+ SDLTest_AssertCheck(result, "Verify result from SDL_FillSurfaceRect() with dst having RLE pixels is true");
+#endif
+
+ /* Make sure the RLE surface still needs to be locked after surface operations */
+ SDLTest_AssertCheck(a->pixels == NULL, "Verify RLE surface after operations does not have pixels available");
+
+ /* Clean up. */
+ SDL_DestroySurface(face);
+ SDL_DestroySurface(a);
+ SDL_DestroySurface(b);
+ SDL_DestroySurface(tmp);
+
+ return TEST_COMPLETED;
+}
+
/**
* Tests surface conversion.
*/
@@ -740,9 +874,7 @@ static int SDLCALL surface_testSurfaceConversion(void *arg)
/* Clean up. */
SDL_DestroySurface(face);
- face = NULL;
SDL_DestroySurface(rface);
- rface = NULL;
return TEST_COMPLETED;
}
@@ -991,9 +1123,9 @@ static int SDLCALL surface_testBlitInvalid(void *arg)
SDLTest_AssertCheck(invalid->pixels == NULL, "Check surface pixels are NULL");
result = SDL_BlitSurface(invalid, NULL, valid, NULL);
- SDLTest_AssertCheck(result == true, "SDL_BlitSurface(invalid, NULL, valid, NULL), result = %s\n", result ? "true" : "false");
+ SDLTest_AssertCheck(result == false, "SDL_BlitSurface(invalid, NULL, valid, NULL), result = %s\n", result ? "true" : "false");
result = SDL_BlitSurface(valid, NULL, invalid, NULL);
- SDLTest_AssertCheck(result == true, "SDL_BlitSurface(valid, NULL, invalid, NULL), result = %s\n", result ? "true" : "false");
+ SDLTest_AssertCheck(result == false, "SDL_BlitSurface(valid, NULL, invalid, NULL), result = %s\n", result ? "true" : "false");
result = SDL_BlitSurfaceScaled(invalid, NULL, valid, NULL, SDL_SCALEMODE_NEAREST);
SDLTest_AssertCheck(result == false, "SDL_BlitSurfaceScaled(invalid, NULL, valid, NULL, SDL_SCALEMODE_NEAREST), result = %s\n", result ? "true" : "false");
@@ -1767,6 +1899,14 @@ static const SDLTest_TestCaseReference surfaceTestLoadFailure = {
surface_testLoadFailure, "surface_testLoadFailure", "Tests sprite loading. A failure case.", TEST_ENABLED
};
+static const SDLTest_TestCaseReference surfaceTestNULLPixels = {
+ surface_testSurfaceNULLPixels, "surface_testSurfaceNULLPixels", "Tests surface operations with NULL pixels.", TEST_ENABLED
+};
+
+static const SDLTest_TestCaseReference surfaceTestRLEPixels = {
+ surface_testSurfaceRLEPixels, "surface_testSurfaceRLEPixels", "Tests surface operations with RLE surfaces.", TEST_ENABLED
+};
+
static const SDLTest_TestCaseReference surfaceTestSurfaceConversion = {
surface_testSurfaceConversion, "surface_testSurfaceConversion", "Tests surface conversion.", TEST_ENABLED
};
@@ -1857,6 +1997,8 @@ static const SDLTest_TestCaseReference *surfaceTests[] = {
&surfaceTestBlit9Grid,
&surfaceTestBlitMultiple,
&surfaceTestLoadFailure,
+ &surfaceTestNULLPixels,
+ &surfaceTestRLEPixels,
&surfaceTestSurfaceConversion,
&surfaceTestCompleteSurfaceConversion,
&surfaceTestBlitColorMod,