Amebis/wxWidgets
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix clobbering warnings in PNG image loading code in a better way

Browse Source 
Add wxPNGImageData and use it to store variables that used to be local
in wxPNGHandler::LoadFile() and could be clobbered by the use of
longjmp() and pass these variables to DoLoadPNGFile(), which still uses
longjmp(), whereas LoadFile() doesn't any longer.

In addition to fixing the warning, this allows to use C++ dtors for
freeing memory and makes the code shorter and, arguably, more clear than
the original version (although written in an unusual way) and definitely
more clear than the version with the warning fix.

This reverts commit 99ea06203a ("Fix
clobbering warning") and replaces it with a different solution to the
same problem.
This commit is contained in:
Vadim Zeitlin
2018-07-21 16:00:08 +02:00
parent 82881d6b03
commit e24e9323e0
1 changed files with 95 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

151
src/common/imagpng.cpp
View File

@@ -68,6 +68,8 @@ wxIMPLEMENT_DYNAMIC_CLASS(wxPNGHandler,wxImageHandler);
#endif
#endif
namespace
{
// VS: wxPNGInfoStruct declared below is a hack that needs some explanation.
// First, let me describe what's the problem: libpng uses jmp_buf in
@@ -93,10 +95,57 @@ struct wxPNGInfoStruct
wxInputStream *in;
wxOutputStream *out;
} stream;
};
#define WX_PNG_INFO(png_ptr) ((wxPNGInfoStruct*)png_get_io_ptr(png_ptr))
// This is another helper struct which is used to pass parameters to
// DoLoadPNGFile(). It allows us to use the usual RAII for freeing memory,
// which wouldn't be possible inside DoLoadPNGFile() because it uses
// setjmp/longjmp() functions for error handling, which are incompatible with
// C++ destructors.
struct wxPNGImageData
{
wxPNGImageData()
{
lines = NULL;
numLines = 0;
ok = false;
}
bool Alloc(png_uint_32 width, png_uint_32 height)
{
lines = (unsigned char **)calloc(height, sizeof(unsigned char *));
if ( !lines )
return false;
numLines = height;
for ( unsigned int n = 0; n < numLines; n++ )
{
lines[n] = (unsigned char *)malloc( (size_t)(width * 4));
if ( !lines[n] )
return false;
}
return true;
}
~wxPNGImageData()
{
for ( unsigned int n = 0; n < numLines; n++ )
free( lines[n] );
free( lines );
}
unsigned char** lines;
png_uint_32 numLines;
bool ok;
};
} // anonymous namespace
// ----------------------------------------------------------------------------
// helper functions
// ----------------------------------------------------------------------------
@@ -247,27 +296,23 @@ void CopyDataFromPNG(wxImage *image,
#pragma warning(disable:4611)
#endif /* VC++ */
bool
wxPNGHandler::LoadFile(wxImage *image,
wxInputStream& stream,
bool verbose,
int WXUNUSED(index))
// This function uses wxPNGImageData to store some of its "local" variables in
// order to avoid clobbering these variables by longjmp(): having them inside
// the stack frame of the caller prevents this from happening. It also
// "returns" its result via "data" parameter: use its "ok" field to check
// whether loading succeeded or failed.
static
void
DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo, wxPNGImageData& data)
{
// VZ: as this function uses setjmp() the only fool-proof error handling
// method is to use goto (setjmp is not really C++ dtors friendly...)
// allocate the lines pointer dynamically to avoid clobbering by longjmp
unsigned char *** const lines_p = new unsigned char ** (NULL);
unsigned char ** &lines = *lines_p;
png_infop info_ptr = (png_infop) NULL;
wxPNGInfoStruct wxinfo;
png_uint_32 i, width, height = 0;
png_uint_32 width, height = 0;
int bit_depth, color_type, interlace_type;
wxinfo.verbose = verbose;
wxinfo.stream.in = &stream;
image->Destroy();
png_structp png_ptr = png_create_read_struct
@@ -278,7 +323,7 @@ wxPNGHandler::LoadFile(wxImage *image,
wx_PNG_warning
);
if (!png_ptr)
goto error;
return;
// NB: please see the comment near wxPNGInfoStruct declaration for
// explanation why this line is mandatory
@@ -286,7 +331,10 @@ wxPNGHandler::LoadFile(wxImage *image,
info_ptr = png_create_info_struct( png_ptr );
if (!info_ptr)
goto error;
{
png_destroy_read_struct( &png_ptr, (png_infopp) NULL, (png_infopp) NULL );
return;
}
if (setjmp(wxinfo.jmpbuf))
goto error;
@@ -314,17 +362,10 @@ wxPNGHandler::LoadFile(wxImage *image,
// initialize all line pointers to NULL to ensure that they can be safely
// free()d if an error occurs before all of them could be allocated
lines = (unsigned char **)calloc(height, sizeof(unsigned char *));
if ( !lines )
if ( !data.Alloc(width, height) )
goto error;
for (i = 0; i < height; i++)
{
if ((lines[i] = (unsigned char *)malloc( (size_t)(width * 4))) == NULL)
goto error;
}
png_read_image( png_ptr, lines );
png_read_image( png_ptr, data.lines );
png_read_end( png_ptr, info_ptr );
#if wxUSE_PALETTE
@@ -394,49 +435,47 @@ wxPNGHandler::LoadFile(wxImage *image,
}
png_destroy_read_struct( &png_ptr, &info_ptr, (png_infopp) NULL );
// loaded successfully, now init wxImage with this data
CopyDataFromPNG(image, lines, width, height, color_type);
CopyDataFromPNG(image, data.lines, width, height, color_type);
for ( i = 0; i < height; i++ )
free( lines[i] );
free( lines );
delete lines_p;
return true;
// This will indicate to the caller that loading succeeded.
data.ok = true;
error:
if (verbose)
{
wxLogError(_("Couldn't load a PNG image - file is corrupted or not enough memory."));
}
// Note that we only get here if both png_ptr and info_ptr are valid,
// otherwise we just return from this function early.
png_destroy_read_struct( &png_ptr, &info_ptr, (png_infopp) NULL );
}
if ( image->IsOk() )
{
image->Destroy();
}
bool
wxPNGHandler::LoadFile(wxImage *image,
wxInputStream& stream,
bool verbose,
int WXUNUSED(index))
{
wxPNGInfoStruct wxinfo;
wxinfo.verbose = verbose;
wxinfo.stream.in = &stream;
if ( lines )
{
for ( unsigned int n = 0; n < height; n++ )
free( lines[n] );
wxPNGImageData data;
DoLoadPNGFile(image, wxinfo, data);
free( lines );
}
delete lines_p;
if ( png_ptr )
if ( !data.ok )
{
if ( info_ptr )
if (verbose)
{
png_destroy_read_struct( &png_ptr, &info_ptr, (png_infopp) NULL );
free(info_ptr);
wxLogError(_("Couldn't load a PNG image - file is corrupted or not enough memory."));
}
else
png_destroy_read_struct( &png_ptr, (png_infopp) NULL, (png_infopp) NULL );
if ( image->IsOk() )
{
image->Destroy();
}
return false;
}
return false;
return true;
}
// ----------------------------------------------------------------------------