longjmp/setjmp clobbered warning in IMG_png.c

[madebr]

The following errors appear on ci:

 D:/a/SDL_image/SDL_image/src/IMG_png.c:580:16: error: variable 'color_ptr' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]
  580 |     png_colorp color_ptr = NULL;
      |                ^~~~~~~~~
D:/a/SDL_image/SDL_image/src/IMG_png.c:582:18: error: variable 'source' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]
  582 |     SDL_Surface *source = surface;
      |                  ^~~~~~
D:/a/SDL_image/SDL_image/src/IMG_png.c:584:9: error: variable 'png_color_type' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]
  584 |     int png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
      |         ^~~~~~~~~~~~~~

https://github.com/libsdl-org/SDL_image/actions/runs/7661898175/job/20882153620#step:13:29

https://github.com/libsdl-org/SDL_image/blob/abbf3a2addfe8ca9fdd3dd3e1551c14940f0f5e8/src/IMG_png.c#L580-L605

[sezero]

In addition to those, gcc-4.9 also warns for row_pointers and surface in IMG_LoadPNG_RW (lines 240 and 247) for me:

/tmp/SDL_image/src/IMG_png.c: In function 'IMG_SavePNG_RW_libpng':
/tmp/SDL_image/src/IMG_png.c:580:16: warning: variable 'color_ptr' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
     png_colorp color_ptr = NULL;
                ^
/tmp/SDL_image/src/IMG_png.c:584:9: warning: variable 'png_color_type' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
     int png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
         ^
/tmp/SDL_image/src/IMG_png.c: In function 'IMG_LoadPNG_RW':
/tmp/SDL_image/src/IMG_png.c:240:18: warning: variable 'surface' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
     SDL_Surface *surface;
                  ^
/tmp/SDL_image/src/IMG_png.c:247:16: warning: variable 'row_pointers' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
     png_bytep *row_pointers;
                ^

[sezero]

~Warning from IMG_SavePNG_RW_libpng is easy to fix:~

```diff diff --git a/src/IMG_png.c b/src/IMG_png.c index d09e312..c5727ba 100644 --- a/src/IMG_png.c +++ b/src/IMG_png.c @@ -577,11 +577,11 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) { png_structp png_ptr; png_infop info_ptr; - png_colorp color_ptr = NULL; + png_colorp color_ptr; Uint8 transparent_table[256]; - SDL_Surface *source = surface; + SDL_Surface *source; SDL_Palette *palette; - int png_color_type = PNG_COLOR_TYPE_RGB_ALPHA; + int png_color_type; if (!IMG_Init(IMG_INIT_PNG)) { return -1; @@ -609,6 +609,10 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) return IMG_SetError("Error writing the PNG file."); } + color_ptr = NULL; + source = surface; + png_color_type = PNG_COLOR_TYPE_RGB_ALPHA; + palette = surface->format->palette; if (palette) { const int ncolors = palette->ncolors; ```

~Not exactly sure about what to do with the ones from IMG_LoadPNG_RW~ ~because of the goto done clean-ups.~

[slouken]

Does adding the volatile keyword to the cleanup variables work?

[sezero]

Does adding the volatile keyword to the cleanup variables work?

Tried adding volatile to surface and row_pointers at lines 240 and 247: no, doesn't eliminate those two -Wclobbered, and adds new discards 'volatile' qualifier warnings.

[slouken]

What if those variables are stored in a malloc'd structure, allocated at the top of the function?

My understanding is that warning is telling us that any changes to the stack variables could be undone when the longjmp returns. But if we allocate a pointer at the beginning and free it at the end and reference the malloced memory in between, we should avoid that.

[sezero]

Well, your first reaction about volatile use was correct: I seem to have placed volatile wrongly and it does help: I pushed https://github.com/libsdl-org/SDL_image/commit/a4ae71897bcf1d8320a9bad9d39362d3977322e1 (which is a partial revert of https://github.com/libsdl-org/SDL_image/commit/a61c2fc38f768b509d204182b4e988669036eb14) and https://github.com/libsdl-org/SDL_image/commit/5dc391d750e63711f2ebd6f44bd193e6845fd06f which silences the warnings. I.e. the final result turned into this:

diff --git a/src/IMG_png.c b/src/IMG_png.c
index d09e312..640cc5b 100644
--- a/src/IMG_png.c
+++ b/src/IMG_png.c
@@ -237,14 +237,14 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src)
 {
     Sint64 start;
     const char *error;
-    SDL_Surface *surface;
+    SDL_Surface *volatile surface;
     png_structp png_ptr;
     png_infop info_ptr;
     png_uint_32 width, height;
     int bit_depth, color_type, interlace_type, num_channels;
     Uint32 format;
     SDL_Palette *palette;
-    png_bytep *row_pointers;
+    png_bytep *volatile row_pointers;
     int row, i;
     int ckey;
     png_color_16 *transv;
@@ -577,11 +577,11 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
 {
     png_structp png_ptr;
     png_infop info_ptr;
-    png_colorp color_ptr = NULL;
+    png_colorp volatile color_ptr = NULL;
     Uint8 transparent_table[256];
-    SDL_Surface *source = surface;
+    SDL_Surface * volatile source = surface;
     SDL_Palette *palette;
-    int png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
+    int png_color_type;

     if (!IMG_Init(IMG_INIT_PNG)) {
         return -1;
@@ -652,6 +652,7 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
     else if (surface->format->format != png_format) {
         /* Otherwise, (surface has alpha data), and it is not in the exact right
            format , so it should be converted to that */
+        png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
         source = SDL_ConvertSurfaceFormat(surface, png_format);
     }

Is this correct? If it is, then we can close this ticket.

P.S.: IMG_SavePNG_RW_libpng() parts may go into SDL2 branch too.

[sezero]

(CC: @1bsyl about the partial revert of https://github.com/libsdl-org/SDL_image/commit/a61c2fc38f768b509d204182b4e988669036eb1)

[1bsyl]

setjmp() isn't really nice to read. In the code, it means, upon error, the libpng functions can go back to the jump position: in the failing section: setjmp() {... goto done; }.

I can't get the same error with "Werror=clobbered" (only IMG_SavePNG_RW_libpng has the error.) ...

maybe repeating the initialization after setjmp() is ok to fix the warning:

diff --git a/src/IMG_png.c b/src/IMG_png.c
index 640cc5b..00828ee 100644
--- a/src/IMG_png.c
+++ b/src/IMG_png.c
@@ -237,14 +237,14 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src)
 {
     Sint64 start;
     const char *error;
-    SDL_Surface *volatile surface;
+    SDL_Surface *surface;
     png_structp png_ptr;
     png_infop info_ptr;
     png_uint_32 width, height;
     int bit_depth, color_type, interlace_type, num_channels;
     Uint32 format;
     SDL_Palette *palette;
-    png_bytep *volatile row_pointers;
+    png_bytep *row_pointers;
     int row, i;
     int ckey;
     png_color_16 *transv;
@@ -297,6 +297,9 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src)
         goto done;
     }
 #endif
+
+    row_pointers = NULL; surface = NULL;
+
     /* Set up the input control */
     lib.png_set_read_fn(png_ptr, src, png_read_data);

@@ -577,9 +580,9 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
 {
     png_structp png_ptr;
     png_infop info_ptr;
-    png_colorp volatile color_ptr = NULL;
+    png_colorp color_ptr = NULL;
     Uint8 transparent_table[256];
-    SDL_Surface * volatile source = surface;
+    SDL_Surface * source = surface;
     SDL_Palette *palette;
     int png_color_type;

@@ -609,6 +612,11 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
         return IMG_SetError("Error writing the PNG file.");
     }

+    source = surface;
+    color_ptr = NULL;
+    png_color_type = PNG_COLOR_TYPE_RGB;
+
+
     palette = surface->format->palette;
     if (palette) {
         const int ncolors = palette->ncolors;

[sezero]

setjmp() isn't really nice to read.

Indeed, and it's a mess, ...

In the code, it means, upon error, the libpng functions can go back to the jump position: in the failing section: setjmp() {... goto done; }.

maybe repeating the initialization after setjmp() is ok to fix the warning:

Haven't tried that way, and it really may silence the warning. However upon rewinding the stack, we will already be leaking the allocated memory, I don't know exactly how volatile is silencing gcc here, and I don't know a better way of handling this mess: libpng doesn't seem to return error but rely on longjmp as far as I can see??..

[1bsyl]

I think it relies on longjmp() so the parser can cleanly fail at any time.

my understanding is: 1/ you set the longjmp(). and the first time it returns 0... that starts. then, any call to the libpng that fail will return to the first setjmp() call. but with a return code of != 0. so that the block{...}is executed. And the block contains either "returns SDL_SetError or goto done;", the "repeated initialization" won't be re-used and it won't leak anything.

[sezero]

And row_pointers for e.g., won't be leaked upon longjmp, I mean shall it not be clobbered back to NULL?

[sezero]

Anyways, I'm confused enough today :) Leaving this thing to you guys @1bsyl, @slouken, @icculus for now

[1bsyl]

And row_pointers for e.g., won't be leaked upon longjmp, I mean shall it not be clobbered back to NULL

the one from "IMG_SavePNG_RW_libpng" ?? I think, here. it will leak if the setjmp fails in lib.png_write_png() for instance. because, it will returns to setjmp(). and won't call SDL_Free for this local variable (because it's not in the main block). (but maybe png_write_png cannot fails to setjmp())

my understanding is that the stack before setjmp() shouldn't be cleared when the program gets back to setjmp.

[sezero]

Then we can implement @slouken's suggestion somehow, i.e.: create a struct with necessary vars, make the libpng-calling procedure into a helper and pass that struct pointer to it? I might mess with that today later.

[1bsyl]

I think it's fine to have the volatile ! but IMG_SavePNG_RW_libpng seems to totally wrong since nothing get freed when a setjump error occur

[1bsyl]

would done something like this:

diff --git a/src/IMG_png.c b/src/IMG_png.c
index 640cc5b..c237ac3 100644
--- a/src/IMG_png.c
+++ b/src/IMG_png.c
@@ -577,11 +577,13 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
 {
     png_structp png_ptr;
     png_infop info_ptr;
+    const char *error;
     png_colorp volatile color_ptr = NULL;
     Uint8 transparent_table[256];
     SDL_Surface * volatile source = surface;
     SDL_Palette *palette;
     int png_color_type;
+    png_bytep *row_pointers = NULL;

     if (!IMG_Init(IMG_INIT_PNG)) {
         return -1;
@@ -605,8 +607,8 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
 #endif
 #endif
     {
-        lib.png_destroy_write_struct(&png_ptr, &info_ptr);
-        return IMG_SetError("Error writing the PNG file.");
+        error = "Error writing the PNG file.";
+        goto done;
     }

     palette = surface->format->palette;
@@ -618,8 +620,8 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
         color_ptr = (png_colorp)SDL_malloc(sizeof(png_color) * ncolors);
         if (color_ptr == NULL)
         {
-            lib.png_destroy_write_struct(&png_ptr, &info_ptr);
-            return IMG_SetError("Couldn't create palette for PNG file");
+            error = "Couldn't create palette for PNG file";
+            goto done;
         }
         for (i = 0; i < ncolors; i++) {
             color_ptr[i].red = palette->colors[i].r;
@@ -663,14 +665,11 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)
                      PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);

     if (source) {
-        png_bytep *row_pointers;
         int row;
-
         row_pointers = (png_bytep *) SDL_malloc(sizeof(png_bytep) * source->h);
         if (!row_pointers) {
-            SDL_free(color_ptr);
-            lib.png_destroy_write_struct(&png_ptr, &info_ptr);
-            return IMG_SetError("Out of memory");
+            error = "Out of memory";
+            goto done;
         }
         for (row = 0; row < (int)source->h; row++) {
             row_pointers[row] = (png_bytep) (Uint8 *) source->pixels + row * source->pitch;
@@ -678,16 +677,31 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst)

         lib.png_set_rows(png_ptr, info_ptr, row_pointers);
         lib.png_write_png(png_ptr, info_ptr, PNG_TRANSFORM_IDENTITY, NULL);
+    }

-        SDL_free(row_pointers);
-        if (source != surface) {
-            SDL_DestroySurface(source);
-        }
+done:/* Clean up and return */
+
+    if (png_ptr) {
+        lib.png_destroy_write_struct(&png_ptr, &info_ptr);
     }
-    lib.png_destroy_write_struct(&png_ptr, &info_ptr);
+
     if (color_ptr) {
         SDL_free(color_ptr);
     }
+
+    if (row_pointers) {
+        SDL_free(row_pointers);
+    }
+
+    if (source != surface) {
+        SDL_DestroySurface(source);
+    }
+
+    if (error) {
+        IMG_SetError("%s", error);
+        return -1;
+    }
+
     return 0;
 }

[1bsyl]

here's a PR: https://github.com/libsdl-org/SDL_image/pull/417 only test that it correctly save png file

[1bsyl]

And bug fixed: uninitialized color type when saving SDL_PIXELFORMAT_RGBA32 surface

[sezero]

Alternative patch: patch-416.txt Generated after applying your #417 and splitting the procedures. Should handle longjmp funnies, I hope. Can you review?

@slouken: Is this close to your suggestion above?

Inlining below for convenience: What do you think?

```diff diff --git a/src/IMG_png.c b/src/IMG_png.c index 640cc5b..6b318b0 100644 --- a/src/IMG_png.c +++ b/src/IMG_png.c @@ -233,49 +233,38 @@ static void png_read_data(png_structp ctx, png_bytep area, png_size_t size) src = (SDL_RWops *)lib.png_get_io_ptr(ctx); SDL_RWread(src, area, size); } -SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) -{ - Sint64 start; + +struct loadpng_vars { const char *error; - SDL_Surface *volatile surface; + SDL_Surface *surface; png_structp png_ptr; png_infop info_ptr; + png_bytep *row_pointers; +}; + +static void IMG_LoadPNG_RW_impl(SDL_RWops *src, struct loadpng_vars *vars) +{ png_uint_32 width, height; int bit_depth, color_type, interlace_type, num_channels; Uint32 format; SDL_Palette *palette; - png_bytep *volatile row_pointers; int row, i; int ckey; png_color_16 *transv; - if ( !src ) { - /* The error message has been set in SDL_RWFromFile */ - return NULL; - } - start = SDL_RWtell(src); - - if ( (IMG_Init(IMG_INIT_PNG) & IMG_INIT_PNG) == 0 ) { - return NULL; - } - - /* Initialize the data we will clean up when we're done */ - error = NULL; - png_ptr = NULL; info_ptr = NULL; row_pointers = NULL; surface = NULL; - /* Create the PNG loading context structure */ - png_ptr = lib.png_create_read_struct(PNG_LIBPNG_VER_STRING, + vars->png_ptr = lib.png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL,NULL,NULL); - if (png_ptr == NULL){ - error = "Couldn't allocate memory for PNG file or incompatible PNG dll"; - goto done; + if (vars->png_ptr == NULL) { + vars->error = "Couldn't allocate memory for PNG file or incompatible PNG dll"; + return; } /* Allocate/initialize the memory for image information. REQUIRED. */ - info_ptr = lib.png_create_info_struct(png_ptr); - if (info_ptr == NULL) { - error = "Couldn't create image information for PNG file"; - goto done; + vars->info_ptr = lib.png_create_info_struct(vars->png_ptr); + if (vars->info_ptr == NULL) { + vars->error = "Couldn't create image information for PNG file"; + return; } /* Set error handling if you are using setjmp/longjmp method (this is @@ -288,46 +277,46 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) #pragma warning(disable:4611) /* warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable */ #endif #ifndef LIBPNG_VERSION_12 - if ( setjmp(*lib.png_set_longjmp_fn(png_ptr, longjmp, sizeof (jmp_buf))) ) + if (setjmp(*lib.png_set_longjmp_fn(vars->png_ptr, longjmp, sizeof(jmp_buf)))) #else - if ( setjmp(png_ptr->jmpbuf) ) + if (setjmp(vars->png_ptr->jmpbuf)) #endif { - error = "Error reading the PNG file."; - goto done; + vars->error = "Error reading the PNG file."; + return; } #endif /* Set up the input control */ - lib.png_set_read_fn(png_ptr, src, png_read_data); + lib.png_set_read_fn(vars->png_ptr, src, png_read_data); /* Read PNG header info */ - lib.png_read_info(png_ptr, info_ptr); - lib.png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, + lib.png_read_info(vars->png_ptr, vars->info_ptr); + lib.png_get_IHDR(vars->png_ptr, vars->info_ptr, &width, &height, &bit_depth, &color_type, &interlace_type, NULL, NULL); /* tell libpng to strip 16 bit/color files down to 8 bits/color */ - lib.png_set_strip_16(png_ptr); + lib.png_set_strip_16(vars->png_ptr); /* tell libpng to de-interlace (if the image is interlaced) */ - lib.png_set_interlace_handling(png_ptr); + lib.png_set_interlace_handling(vars->png_ptr); /* Extract multiple pixels with bit depths of 1, 2, and 4 from a single * byte into separate bytes (useful for paletted and grayscale images). */ - lib.png_set_packing(png_ptr); + lib.png_set_packing(vars->png_ptr); /* scale greyscale values to the range 0..255 */ if (color_type == PNG_COLOR_TYPE_GRAY) - lib.png_set_expand(png_ptr); + lib.png_set_expand(vars->png_ptr); /* For images with a single "transparent colour", set colour key; if more than one index has transparency, or if partially transparent entries exist, use full alpha channel */ ckey = -1; - if (lib.png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) { + if (lib.png_get_valid(vars->png_ptr, vars->info_ptr, PNG_INFO_tRNS)) { int num_trans; Uint8 *trans; - lib.png_get_tRNS(png_ptr, info_ptr, &trans, &num_trans, &transv); + lib.png_get_tRNS(vars->png_ptr, vars->info_ptr, &trans, &num_trans, &transv); if (color_type == PNG_COLOR_TYPE_PALETTE) { /* Check if all tRNS entries are opaque except one */ int j, t = -1; @@ -346,7 +335,7 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) ckey = t; } else { /* more than one transparent index, or translucency */ - lib.png_set_expand(png_ptr); + lib.png_set_expand(vars->png_ptr); } } else { ckey = 0; /* actual value will be set later */ @@ -354,15 +343,15 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) } if ( color_type == PNG_COLOR_TYPE_GRAY_ALPHA ) - lib.png_set_gray_to_rgb(png_ptr); + lib.png_set_gray_to_rgb(vars->png_ptr); - lib.png_read_update_info(png_ptr, info_ptr); + lib.png_read_update_info(vars->png_ptr, vars->info_ptr); - lib.png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, + lib.png_get_IHDR(vars->png_ptr, vars->info_ptr, &width, &height, &bit_depth, &color_type, &interlace_type, NULL, NULL); /* Allocate the SDL surface to hold the image */ - num_channels = lib.png_get_channels(png_ptr, info_ptr); + num_channels = lib.png_get_channels(vars->png_ptr, vars->info_ptr); format = SDL_PIXELFORMAT_UNKNOWN; if (num_channels == 3) { @@ -396,36 +385,36 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) } } - surface = SDL_CreateSurface(width, height, format); - if ( surface == NULL ) { - error = SDL_GetError(); - goto done; + vars->surface = SDL_CreateSurface(width, height, format); + if (vars->surface == NULL) { + vars->error = SDL_GetError(); + return; } if (ckey != -1) { if (color_type != PNG_COLOR_TYPE_PALETTE) { /* FIXME: Should these be truncated or shifted down? */ - ckey = SDL_MapRGB(surface->format, + ckey = SDL_MapRGB(vars->surface->format, (Uint8)transv->red, (Uint8)transv->green, (Uint8)transv->blue); } - SDL_SetSurfaceColorKey(surface, SDL_TRUE, ckey); + SDL_SetSurfaceColorKey(vars->surface, SDL_TRUE, ckey); } /* Create the array of pointers to image data */ - row_pointers = (png_bytep*) SDL_malloc(sizeof(png_bytep)*height); - if (!row_pointers) { - error = "Out of memory"; - goto done; + vars->row_pointers = (png_bytep*) SDL_malloc(sizeof(png_bytep)*height); + if (!vars->row_pointers) { + vars->error = "Out of memory"; + return; } for (row = 0; row < (int)height; row++) { - row_pointers[row] = (png_bytep) - (Uint8 *)surface->pixels + row*surface->pitch; + vars->row_pointers[row] = (png_bytep) + (Uint8 *)vars->surface->pixels + row*vars->surface->pitch; } /* Read the entire image in one go */ - lib.png_read_image(png_ptr, row_pointers); + lib.png_read_image(vars->png_ptr, vars->row_pointers); /* and we're done! (png_read_end() can be omitted if no processing of * post-IDAT text/time/etc. is desired) @@ -436,11 +425,11 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) */ /* Load the palette, if any */ - palette = surface->format->palette; + palette = vars->surface->format->palette; if ( palette ) { int png_num_palette; png_colorp png_palette; - lib.png_get_PLTE(png_ptr, info_ptr, &png_palette, &png_num_palette); + lib.png_get_PLTE(vars->png_ptr, vars->info_ptr, &png_palette, &png_num_palette); if (color_type == PNG_COLOR_TYPE_GRAY) { palette->ncolors = 256; for (i = 0; i < 256; i++) { @@ -457,25 +446,45 @@ SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) } } } +} + +SDL_Surface *IMG_LoadPNG_RW(SDL_RWops *src) +{ + Sint64 start; + struct loadpng_vars vars; + + if ( !src ) { + /* The error message has been set in SDL_RWFromFile */ + return NULL; + } + start = SDL_RWtell(src); + + if ( (IMG_Init(IMG_INIT_PNG) & IMG_INIT_PNG) == 0 ) { + return NULL; + } + + SDL_zero(vars); -done: /* Clean up and return */ - if ( png_ptr ) { - lib.png_destroy_read_struct(&png_ptr, - info_ptr ? &info_ptr : (png_infopp)0, + IMG_LoadPNG_RW_impl(src, &vars); + + if (vars.png_ptr) { + lib.png_destroy_read_struct(&vars.png_ptr, + vars.info_ptr ? &vars.info_ptr : (png_infopp)0, (png_infopp)0); } - if ( row_pointers ) { - SDL_free(row_pointers); + if (vars.row_pointers) { + SDL_free(vars.row_pointers); } - if ( error ) { + if (vars.error) { SDL_RWseek(src, start, SDL_RW_SEEK_SET); - if ( surface ) { - SDL_DestroySurface(surface); - surface = NULL; + if (vars.surface) { + SDL_DestroySurface(vars.surface); + vars.surface = NULL; } - IMG_SetError("%s", error); + IMG_SetError("%s", vars.error); } - return(surface); + + return vars.surface; } #elif defined(USE_STBIMAGE) @@ -573,40 +582,47 @@ static void png_flush_data(png_structp png_ptr) (void)png_ptr; } -static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) -{ +struct savepng_vars { png_structp png_ptr; png_infop info_ptr; - png_colorp volatile color_ptr = NULL; + const char *error; + png_colorp color_ptr; + png_bytep *row_pointers; + SDL_Surface *source; +}; + +static int IMG_SavePNG_RW_libpng_impl(struct savepng_vars *vars, SDL_Surface *surface, SDL_RWops *dst) +{ Uint8 transparent_table[256]; - SDL_Surface * volatile source = surface; SDL_Palette *palette; int png_color_type; + vars->source = surface; + if (!IMG_Init(IMG_INIT_PNG)) { return -1; } - png_ptr = lib.png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL); - if (png_ptr == NULL) { + vars->png_ptr = lib.png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL); + if (vars->png_ptr == NULL) { return IMG_SetError("Couldn't allocate memory for PNG file or incompatible PNG dll"); } - info_ptr = lib.png_create_info_struct(png_ptr); - if (info_ptr == NULL) { - lib.png_destroy_write_struct(&png_ptr, NULL); - return IMG_SetError("Couldn't create image information for PNG file"); + vars->info_ptr = lib.png_create_info_struct(vars->png_ptr); + if (vars->info_ptr == NULL) { + vars->error = "Couldn't create image information for PNG file"; + return -1; } #ifdef PNG_SETJMP_SUPPORTED #ifndef LIBPNG_VERSION_12 - if (setjmp(*lib.png_set_longjmp_fn(png_ptr, longjmp, sizeof (jmp_buf)))) + if (setjmp(*lib.png_set_longjmp_fn(vars->png_ptr, longjmp, sizeof (jmp_buf)))) #else - if (setjmp(png_ptr->jmpbuf)) + if (setjmp(vars->png_ptr->jmpbuf)) #endif #endif { - lib.png_destroy_write_struct(&png_ptr, &info_ptr); - return IMG_SetError("Error writing the PNG file."); + vars->error = "Error writing the PNG file."; + return -1; } palette = surface->format->palette; @@ -615,28 +631,27 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) int i; int last_transparent = -1; - color_ptr = (png_colorp)SDL_malloc(sizeof(png_color) * ncolors); - if (color_ptr == NULL) - { - lib.png_destroy_write_struct(&png_ptr, &info_ptr); - return IMG_SetError("Couldn't create palette for PNG file"); + vars->color_ptr = (png_colorp)SDL_malloc(sizeof(png_color) * ncolors); + if (vars->color_ptr == NULL) { + vars->error = "Couldn't create palette for PNG file"; + return -1; } for (i = 0; i < ncolors; i++) { - color_ptr[i].red = palette->colors[i].r; - color_ptr[i].green = palette->colors[i].g; - color_ptr[i].blue = palette->colors[i].b; + vars->color_ptr[i].red = palette->colors[i].r; + vars->color_ptr[i].green = palette->colors[i].g; + vars->color_ptr[i].blue = palette->colors[i].b; if (palette->colors[i].a != 255) { last_transparent = i; } } - lib.png_set_PLTE(png_ptr, info_ptr, color_ptr, ncolors); + lib.png_set_PLTE(vars->png_ptr, vars->info_ptr, vars->color_ptr, ncolors); png_color_type = PNG_COLOR_TYPE_PALETTE; if (last_transparent >= 0) { for (i = 0; i <= last_transparent; ++i) { transparent_table[i] = palette->colors[i].a; } - lib.png_set_tRNS(png_ptr, info_ptr, transparent_table, last_transparent + 1, NULL); + lib.png_set_tRNS(vars->png_ptr, vars->info_ptr, transparent_table, last_transparent + 1, NULL); } } else if (surface->format->format == SDL_PIXELFORMAT_RGB24) { @@ -647,48 +662,70 @@ static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) /* If the surface is not exactly the right RGB format but does not have alpha information, it should be converted to RGB24 before being passed through */ png_color_type = PNG_COLOR_TYPE_RGB; - source = SDL_ConvertSurfaceFormat(surface, SDL_PIXELFORMAT_RGB24); + vars->source = SDL_ConvertSurfaceFormat(surface, SDL_PIXELFORMAT_RGB24); } else if (surface->format->format != png_format) { /* Otherwise, (surface has alpha data), and it is not in the exact right format , so it should be converted to that */ png_color_type = PNG_COLOR_TYPE_RGB_ALPHA; - source = SDL_ConvertSurfaceFormat(surface, png_format); + vars->source = SDL_ConvertSurfaceFormat(surface, png_format); + } else { + png_color_type = PNG_COLOR_TYPE_RGB_ALPHA; } - lib.png_set_write_fn(png_ptr, dst, png_write_data, png_flush_data); + lib.png_set_write_fn(vars->png_ptr, dst, png_write_data, png_flush_data); - lib.png_set_IHDR(png_ptr, info_ptr, surface->w, surface->h, + lib.png_set_IHDR(vars->png_ptr, vars->info_ptr, surface->w, surface->h, 8, png_color_type, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT); - if (source) { - png_bytep *row_pointers; + if (vars->source) { int row; - - row_pointers = (png_bytep *) SDL_malloc(sizeof(png_bytep) * source->h); - if (!row_pointers) { - SDL_free(color_ptr); - lib.png_destroy_write_struct(&png_ptr, &info_ptr); - return IMG_SetError("Out of memory"); + vars->row_pointers = (png_bytep *) SDL_malloc(sizeof(png_bytep) * vars->source->h); + if (!vars->row_pointers) { + vars->error = "Out of memory"; + return -1; } - for (row = 0; row < (int)source->h; row++) { - row_pointers[row] = (png_bytep) (Uint8 *) source->pixels + row * source->pitch; + for (row = 0; row < (int)vars->source->h; row++) { + vars->row_pointers[row] = (png_bytep) (Uint8 *) vars->source->pixels + row * vars->source->pitch; } - lib.png_set_rows(png_ptr, info_ptr, row_pointers); - lib.png_write_png(png_ptr, info_ptr, PNG_TRANSFORM_IDENTITY, NULL); + lib.png_set_rows(vars->png_ptr, vars->info_ptr, vars->row_pointers); + lib.png_write_png(vars->png_ptr, vars->info_ptr, PNG_TRANSFORM_IDENTITY, NULL); + } - SDL_free(row_pointers); - if (source != surface) { - SDL_DestroySurface(source); - } + return 0; +} + +static int IMG_SavePNG_RW_libpng(SDL_Surface *surface, SDL_RWops *dst) +{ + struct savepng_vars vars; + int ret; + + SDL_zero(vars); + ret = IMG_SavePNG_RW_libpng_impl(&vars, surface, dst); + + if (vars.png_ptr) { + lib.png_destroy_write_struct(&vars.png_ptr, &vars.info_ptr); } - lib.png_destroy_write_struct(&png_ptr, &info_ptr); - if (color_ptr) { - SDL_free(color_ptr); + + if (vars.color_ptr) { + SDL_free(vars.color_ptr); } - return 0; + + if (vars.row_pointers) { + SDL_free(vars.row_pointers); + } + + if (vars.source != surface) { + SDL_DestroySurface(vars.source); + } + + if (vars.error) { + IMG_SetError("%s", vars.error); + } + + return ret; } #endif /* USE_LIBPNG */ ```

[slouken]

Do both of these patches resolve the setjmp/longjmp warnings?

[slouken]

i.e. is @1bsyl's patch in #416 enough?

[sezero]

Do both of these patches resolve the setjmp/longjmp warnings?

Yes. Created PR #418 now to ensure it. Let's see how it goes. (Please review it, btw.)

[sezero]

i.e. is @1bsyl's patch in #416 enough?

AFAICS, it doesn't handle leaks upon a longjmp (but it's just me.)

[sezero]

Fixed by #418, closing.