surface: verify surface palette in SDL_Save(BMP|PNG) before hitting the FS

src/video/SDL_bmp.c

@@ -591,6 +591,78 @@ done:
     return surface;
 }
 
+typedef struct {
+    SDL_Surface *surface;
+    SDL_Surface *intermediate_surface;
+    bool save32bit;
+    bool saveLegacyBMP;
+} BMPSaveState;
+
+static void FreeBMPSaveState(BMPSaveState *state)
+{
+    if (state->intermediate_surface && state->intermediate_surface != state->surface) {
+        SDL_DestroySurface(state->intermediate_surface);
+    }
+}
+
+static bool InitBMPSaveState(BMPSaveState *state, SDL_Surface *surface)
+{
+    state->surface = surface;
+    state->intermediate_surface = NULL;
+    state->save32bit = false;
+    state->saveLegacyBMP = false;
+
+    CHECK_PARAM(!SDL_SurfaceValid(surface)) {
+        return SDL_InvalidParamError("surface");
+    }
+
+#ifdef SAVE_32BIT_BMP
+    // We can save alpha information in a 32-bit BMP
+    if (SDL_BITSPERPIXEL(surface->format) >= 8 &&
+        (SDL_ISPIXELFORMAT_ALPHA(surface->format) ||
+         (surface->map.info.flags & SDL_COPY_COLORKEY))) {
+        state->save32bit = true;
+    }
+#endif // SAVE_32BIT_BMP
+
+    if (surface->palette && !state->save32bit) {
+        if (SDL_BITSPERPIXEL(surface->format) == 8) {
+            state->intermediate_surface = surface;
+        } else {
+            SDL_SetError("%u bpp BMP files not supported",
+                         SDL_BITSPERPIXEL(surface->format));
+            goto error;
+        }
+    } else if ((surface->format == SDL_PIXELFORMAT_BGR24 && !state->save32bit) ||
+               (surface->format == SDL_PIXELFORMAT_BGRA32 && state->save32bit)) {
+        state->intermediate_surface = surface;
+    } else {
+        SDL_PixelFormat pixel_format;
+
+        /* If the surface has a colorkey or alpha channel we'll save a
+           32-bit BMP with alpha channel, otherwise save a 24-bit BMP. */
+        if (state->save32bit) {
+            pixel_format = SDL_PIXELFORMAT_BGRA32;
+        } else {
+            pixel_format = SDL_PIXELFORMAT_BGR24;
+        }
+        state->intermediate_surface = SDL_ConvertSurface(surface, pixel_format);
+        if (!state->intermediate_surface) {
+            SDL_SetError("Couldn't convert image to %d bpp",
+                         (int)SDL_BITSPERPIXEL(pixel_format));
+            goto error;
+        }
+    }
+
+    if (state->save32bit) {
+        state->saveLegacyBMP = SDL_GetHintBoolean(SDL_HINT_BMP_SAVE_LEGACY_FORMAT, false);
+    }
+    return true;
+error:
+    FreeBMPSaveState(state);
+    return false;
+}
+
 SDL_Surface *SDL_LoadBMP(const char *file)
 {
     SDL_IOStream *stream = SDL_IOFromFile(file, "rb");
@@ -599,16 +671,12 @@ SDL_Surface *SDL_LoadBMP(const char *file)
     }
     return SDL_LoadBMP_IO(stream, true);
 }
-
+static bool SDL_SaveBMP_IO_Internal(BMPSaveState *state, SDL_IOStream *dst, bool closeio)
-bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
 {
     bool was_error = true;
     Sint64 fp_offset, new_offset;
     int i, pad;
-    SDL_Surface *intermediate_surface = NULL;
     Uint8 *bits;
-    bool save32bit = false;
-    bool saveLegacyBMP = false;
 
     // The Win32 BMP file header (14 bytes)
     char magic[2] = { 'B', 'M' };
@@ -647,60 +715,8 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
     Uint32 bV5ProfileSize = 0;
     Uint32 bV5Reserved = 0;
 
-    // Make sure we have somewhere to save
+    if (SDL_LockSurface(state->intermediate_surface)) {
-    CHECK_PARAM(!SDL_SurfaceValid(surface)) {
+        const size_t bw = state->intermediate_surface->w * state->intermediate_surface->fmt->bytes_per_pixel;
-        SDL_InvalidParamError("surface");
-        goto done;
-    }
-    CHECK_PARAM(!dst) {
-        SDL_InvalidParamError("dst");
-        goto done;
-    }
-
-#ifdef SAVE_32BIT_BMP
-    // We can save alpha information in a 32-bit BMP
-    if (SDL_BITSPERPIXEL(surface->format) >= 8 &&
-        (SDL_ISPIXELFORMAT_ALPHA(surface->format) ||
-         surface->map.info.flags & SDL_COPY_COLORKEY)) {
-        save32bit = true;
-    }
-#endif // SAVE_32BIT_BMP
-
-    if (surface->palette && !save32bit) {
-        if (SDL_BITSPERPIXEL(surface->format) == 8) {
-            intermediate_surface = surface;
-        } else {
-            SDL_SetError("%u bpp BMP files not supported",
-                         SDL_BITSPERPIXEL(surface->format));
-            goto done;
-        }
-    } else if ((surface->format == SDL_PIXELFORMAT_BGR24 && !save32bit) ||
-               (surface->format == SDL_PIXELFORMAT_BGRA32 && save32bit)) {
-        intermediate_surface = surface;
-    } else {
-        SDL_PixelFormat pixel_format;
-
-        /* If the surface has a colorkey or alpha channel we'll save a
-           32-bit BMP with alpha channel, otherwise save a 24-bit BMP. */
-        if (save32bit) {
-            pixel_format = SDL_PIXELFORMAT_BGRA32;
-        } else {
-            pixel_format = SDL_PIXELFORMAT_BGR24;
-        }
-        intermediate_surface = SDL_ConvertSurface(surface, pixel_format);
-        if (!intermediate_surface) {
-            SDL_SetError("Couldn't convert image to %d bpp",
-                         (int)SDL_BITSPERPIXEL(pixel_format));
-            goto done;
-        }
-    }
-
-    if (save32bit) {
-        saveLegacyBMP = SDL_GetHintBoolean(SDL_HINT_BMP_SAVE_LEGACY_FORMAT, false);
-    }
-
-    if (SDL_LockSurface(intermediate_surface)) {
-        const size_t bw = intermediate_surface->w * intermediate_surface->fmt->bytes_per_pixel;
 
         // Set the BMP file header values
         bfSize = 0; // We'll write this when we're done
@@ -723,23 +739,23 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
 
         // Set the BMP info values
         biSize = 40;
-        biWidth = intermediate_surface->w;
+        biWidth = state->intermediate_surface->w;
-        biHeight = intermediate_surface->h;
+        biHeight = state->intermediate_surface->h;
         biPlanes = 1;
-        biBitCount = intermediate_surface->fmt->bits_per_pixel;
+        biBitCount = state->intermediate_surface->fmt->bits_per_pixel;
         biCompression = BI_RGB;
-        biSizeImage = intermediate_surface->h * intermediate_surface->pitch;
+        biSizeImage = state->intermediate_surface->h * state->intermediate_surface->pitch;
         biXPelsPerMeter = 0;
         biYPelsPerMeter = 0;
-        if (intermediate_surface->palette) {
+        if (state->intermediate_surface->palette) {
-            biClrUsed = intermediate_surface->palette->ncolors;
+            biClrUsed = state->intermediate_surface->palette->ncolors;
         } else {
             biClrUsed = 0;
         }
         biClrImportant = 0;
 
         // Set the BMP info values
-        if (save32bit && !saveLegacyBMP) {
+        if (state->save32bit && !state->saveLegacyBMP) {
             biSize = 124;
             // Version 4 values
             biCompression = BI_BITFIELDS;
@@ -775,7 +791,7 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
         }
 
         // Write the BMP info values
-        if (save32bit && !saveLegacyBMP) {
+        if (state->save32bit && !state->saveLegacyBMP) {
             // Version 4 values
             if (!SDL_WriteU32LE(dst, bV4RedMask) ||
                 !SDL_WriteU32LE(dst, bV4GreenMask) ||
@@ -804,12 +820,12 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
         }
 
         // Write the palette (in BGR color order)
-        if (intermediate_surface->palette) {
+        if (state->intermediate_surface->palette) {
             SDL_Color *colors;
             int ncolors;
 
-            colors = intermediate_surface->palette->colors;
+            colors = state->intermediate_surface->palette->colors;
-            ncolors = intermediate_surface->palette->ncolors;
+            ncolors = state->intermediate_surface->palette->ncolors;
             for (i = 0; i < ncolors; ++i) {
                 if (!SDL_WriteU8(dst, colors[i].b) ||
                     !SDL_WriteU8(dst, colors[i].g) ||
@@ -833,10 +849,10 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
         }
 
         // Write the bitmap image upside down
-        bits = (Uint8 *)intermediate_surface->pixels + (intermediate_surface->h * intermediate_surface->pitch);
+        bits = (Uint8 *)state->intermediate_surface->pixels + (state->intermediate_surface->h * state->intermediate_surface->pitch);
         pad = ((bw % 4) ? (4 - (bw % 4)) : 0);
-        while (bits > (Uint8 *)intermediate_surface->pixels) {
+        while (bits > (Uint8 *)state->intermediate_surface->pixels) {
-            bits -= intermediate_surface->pitch;
+            bits -= state->intermediate_surface->pitch;
             if (SDL_WriteIO(dst, bits, bw) != bw) {
                 goto done;
             }
@@ -867,15 +883,12 @@ bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
         }
 
         // Close it up..
-        SDL_UnlockSurface(intermediate_surface);
+        SDL_UnlockSurface(state->intermediate_surface);
 
         was_error = false;
     }
 
 done:
-    if (intermediate_surface && intermediate_surface != surface) {
-        SDL_DestroySurface(intermediate_surface);
-    }
     if (closeio && dst) {
         if (!SDL_CloseIO(dst)) {
             was_error = true;
@@ -887,11 +900,36 @@ done:
     return true;
 }
 
+bool SDL_SaveBMP_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
+{
+    bool result = true;
+    BMPSaveState state;
+    if (!InitBMPSaveState(&state, surface)) {
+        return false;
+    }
+    CHECK_PARAM(!dst) {
+        result = SDL_InvalidParamError("dst");
+        goto done;
+    }
+    result = SDL_SaveBMP_IO_Internal(&state, dst, closeio);
+done:
+    FreeBMPSaveState(&state);
+    return result;
+}
+
 bool SDL_SaveBMP(SDL_Surface *surface, const char *file)
 {
+    BMPSaveState state;
+    if (!InitBMPSaveState(&state, surface)) {
+        return false;
+    }
+
     SDL_IOStream *stream = SDL_IOFromFile(file, "wb");
     if (!stream) {
         return false;
     }
-    return SDL_SaveBMP_IO(surface, stream, true);
+
+    bool result = SDL_SaveBMP_IO_Internal(&state, stream, true);
+    FreeBMPSaveState(&state);
+    return result;
 }

src/video/SDL_stb.c

@@ -395,7 +395,7 @@ bool SDL_SavePNG_IO(SDL_Surface *surface, SDL_IOStream *dst, bool closeio)
     Uint8 *trns = NULL;
     bool free_surface = false;
 
-    // Make sure we have somewhere to save
+    // Make sure we have something to save
     CHECK_PARAM(!SDL_SurfaceValid(surface)) {
         SDL_InvalidParamError("surface");
         goto done;
@@ -478,6 +478,14 @@ done:
 bool SDL_SavePNG(SDL_Surface *surface, const char *file)
 {
 #ifdef SDL_HAVE_STB
+    // Make sure we have something to save
+    CHECK_PARAM(!SDL_SurfaceValid(surface)) {
+        return SDL_InvalidParamError("surface");
+    }
+
+    if (SDL_ISPIXELFORMAT_INDEXED(surface->format) && !surface->palette) {
+        return SDL_SetError("Indexed surfaces must have a palette");
+    }
     SDL_IOStream *stream = SDL_IOFromFile(file, "wb");
     if (!stream) {
         return false;

test/testautomation_surface.c

@@ -328,6 +328,7 @@ static int SDLCALL surface_testSaveLoad(void *arg)
     int ret;
     const char *sampleFilename = "testSaveLoad.tmp";
     SDL_Surface *face;
+    SDL_Surface *indexed_surface;
     SDL_Surface *rface;
     SDL_Palette *palette;
     SDL_Color colors[] = {
@@ -344,6 +345,18 @@ static int SDLCALL surface_testSaveLoad(void *arg)
         return TEST_ABORTED;
     }
 
+    indexed_surface = SDL_CreateSurface(32, 32, SDL_PIXELFORMAT_INDEX8);
+    SDLTest_AssertCheck(indexed_surface != NULL, "SDL_CreateSurface(SDL_PIXELFORMAT_INDEX8)");
+
+    /* Delete test file; ignore errors */
+    SDL_RemovePath(sampleFilename);
+
+    /* Saving an indexed surface without palette as BMP fails */
+    ret = SDL_SaveBMP(indexed_surface, sampleFilename);
+    SDLTest_AssertPass("Call to SDL_SaveBMP() using an indexed surface without palette");
+    SDLTest_AssertCheck(ret == false, "Verify result of SDL_SaveBMP(indexed_surface without palette), expected: false, got: %i", ret);
+    SDLTest_AssertCheck(!SDL_GetPathInfo(sampleFilename, NULL), "No file is created after trying to save a indexed surface without palette");
+
     /* Delete test file; ignore errors */
     SDL_RemovePath(sampleFilename);
 
@@ -367,6 +380,15 @@ static int SDLCALL surface_testSaveLoad(void *arg)
     /* Delete test file; ignore errors */
     SDL_RemovePath(sampleFilename);
 
+    /* Saving an indexed surface as PNG fails */
+    ret = SDL_SavePNG(indexed_surface, sampleFilename);
+    SDLTest_AssertPass("Call to SDL_SavePNG() using an indexed surface without palette");
+    SDLTest_AssertCheck(ret == false, "Verify result of SDL_SavePNG(indexed surface without palette), expected: false, got: %i", ret);
+    SDLTest_AssertCheck(ret == false, "Verify result of SDL_SavePNG(indexed surface without palette), expected: false, got: %i", ret);
+
+    /* Delete test file; ignore errors */
+    SDL_RemovePath(sampleFilename);
+
     /* Save a PNG surface */
     ret = SDL_SavePNG(face, sampleFilename);
     SDLTest_AssertPass("Call to SDL_SavePNG()");
@@ -416,6 +438,9 @@ static int SDLCALL surface_testSaveLoad(void *arg)
     if (stream == NULL) {
         return TEST_ABORTED;
     }
+    ret = SDL_SaveBMP_IO(indexed_surface, stream, false);
+    SDLTest_AssertCheck(ret == false, "Verify result from SDL_SaveBMP (indexed surface without palette), expected: false, got: %i", ret);
+    SDL_SeekIO(stream, 0, SDL_IO_SEEK_SET);
     ret = SDL_SaveBMP_IO(face, stream, false);
     SDLTest_AssertPass("Call to SDL_SaveBMP()");
     SDLTest_AssertCheck(ret == true, "Verify result from SDL_SaveBMP, expected: true, got: %i", ret);
@@ -447,6 +472,9 @@ static int SDLCALL surface_testSaveLoad(void *arg)
     if (stream == NULL) {
         return TEST_ABORTED;
     }
+    ret = SDL_SavePNG_IO(indexed_surface, stream, false);
+    SDLTest_AssertCheck(ret == false, "Verify result from SDL_SavePNG_IO (indexed surface without palette), expected: false, got: %i", ret);
+    SDL_SeekIO(stream, 0, SDL_IO_SEEK_SET);
     ret = SDL_SavePNG_IO(face, stream, false);
     SDLTest_AssertPass("Call to SDL_SavePNG()");
     SDLTest_AssertCheck(ret == true, "Verify result from SDL_SavePNG, expected: true, got: %i", ret);
@@ -472,6 +500,7 @@ static int SDLCALL surface_testSaveLoad(void *arg)
     SDL_CloseIO(stream);
     stream = NULL;
 
+    SDL_DestroySurface(indexed_surface);
     SDL_DestroySurface(face);
 
     return TEST_COMPLETED;