From 99ea06203aec0ebb1f8978cb34a50483e0fa746c Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Tue, 3 Jul 2018 13:33:03 +0300 Subject: [PATCH 1/8] Fix clobbering warning --- src/common/imagpng.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/common/imagpng.cpp b/src/common/imagpng.cpp index 11b63b58fa..0644d0c82d 100644 --- a/src/common/imagpng.cpp +++ b/src/common/imagpng.cpp @@ -256,7 +256,9 @@ wxPNGHandler::LoadFile(wxImage *image, // VZ: as this function uses setjmp() the only fool-proof error handling // method is to use goto (setjmp is not really C++ dtors friendly...) - unsigned char **lines = NULL; + // 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; @@ -400,6 +402,7 @@ wxPNGHandler::LoadFile(wxImage *image, for ( i = 0; i < height; i++ ) free( lines[i] ); free( lines ); + delete lines_p; return true; @@ -421,6 +424,7 @@ error: free( lines ); } + delete lines_p; if ( png_ptr ) { From 2171076e8113fb54dfc128357018f5f9d3f8494e Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Fri, 6 Jul 2018 10:54:15 +0300 Subject: [PATCH 2/8] Avoid pointer overflow warning in wxPropertyGridPageState::DoRemoveFromSelection() Looks like gcc8.1 think 'sel' can be empty after initialization. Avoid this by copying only the remaining entries. --- src/propgrid/propgridpagestate.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/propgrid/propgridpagestate.cpp b/src/propgrid/propgridpagestate.cpp index ad9fdc5858..870354a27f 100644 --- a/src/propgrid/propgridpagestate.cpp +++ b/src/propgrid/propgridpagestate.cpp @@ -1371,8 +1371,7 @@ void wxPropertyGridPageState::DoRemoveFromSelection( wxPGProperty* prop ) { // If first item (i.e. one with the active editor) was // deselected, then we need to take some extra measures. - wxArrayPGProperty sel = m_selection; - sel.erase( sel.begin() + i ); + wxArrayPGProperty sel(m_selection.begin() + 1, m_selection.end()); wxPGProperty* newFirst = sel.empty()? NULL: sel[0]; From 915b5212f8c4e4d026b4ae6c761572cc0a19f79f Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Thu, 12 Jul 2018 11:15:10 +0300 Subject: [PATCH 3/8] Avoid unsafe casts to enum type --- src/generic/statusbr.cpp | 6 +++--- src/msw/statusbar.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/generic/statusbr.cpp b/src/generic/statusbr.cpp index f364677c7b..c0581dd679 100644 --- a/src/generic/statusbr.cpp +++ b/src/generic/statusbr.cpp @@ -231,12 +231,12 @@ void wxStatusBarGeneric::DrawFieldText(wxDC& dc, const wxRect& rect, int i, int // eventually ellipsize the text so that it fits the field width - wxEllipsizeMode ellmode = (wxEllipsizeMode)-1; + wxEllipsizeMode ellmode = wxELLIPSIZE_NONE; if (HasFlag(wxSTB_ELLIPSIZE_START)) ellmode = wxELLIPSIZE_START; else if (HasFlag(wxSTB_ELLIPSIZE_MIDDLE)) ellmode = wxELLIPSIZE_MIDDLE; else if (HasFlag(wxSTB_ELLIPSIZE_END)) ellmode = wxELLIPSIZE_END; - if (ellmode == (wxEllipsizeMode)-1) + if (ellmode == wxELLIPSIZE_NONE) { // if we have the wxSTB_SHOW_TIPS we must set the ellipsized flag even if // we don't ellipsize the text but just truncate it @@ -267,7 +267,7 @@ void wxStatusBarGeneric::DrawFieldText(wxDC& dc, const wxRect& rect, int i, int // draw the text dc.DrawText(text, xpos, ypos); - if (ellmode == (wxEllipsizeMode)-1) + if (ellmode == wxELLIPSIZE_NONE) dc.DestroyClippingRegion(); } diff --git a/src/msw/statusbar.cpp b/src/msw/statusbar.cpp index 52ed30fa79..ad9b4eff96 100644 --- a/src/msw/statusbar.cpp +++ b/src/msw/statusbar.cpp @@ -287,12 +287,12 @@ void wxStatusBar::DoUpdateStatusText(int nField) wxString text = GetStatusText(nField); // do we need to ellipsize this string? - wxEllipsizeMode ellmode = (wxEllipsizeMode)-1; + wxEllipsizeMode ellmode = wxELLIPSIZE_NONE; if (HasFlag(wxSTB_ELLIPSIZE_START)) ellmode = wxELLIPSIZE_START; else if (HasFlag(wxSTB_ELLIPSIZE_MIDDLE)) ellmode = wxELLIPSIZE_MIDDLE; else if (HasFlag(wxSTB_ELLIPSIZE_END)) ellmode = wxELLIPSIZE_END; - if (ellmode == (wxEllipsizeMode)-1) + if (ellmode == wxELLIPSIZE_NONE) { // if we have the wxSTB_SHOW_TIPS we must set the ellipsized flag even if // we don't ellipsize the text but just truncate it From 82881d6b038c10906704f5e53819051d2a9b6a5a Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Thu, 12 Jul 2018 12:19:07 +0300 Subject: [PATCH 4/8] Use memcpy instead of strncpy in wxStrlcpy Avoid stringop-overflow warning about strncpy size argument depending on source size. Checking for null in src is not needed because it is already checked by strlen. --- include/wx/wxcrt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/wx/wxcrt.h b/include/wx/wxcrt.h index 2cbd853ecf..1a3e5e0dbf 100644 --- a/include/wx/wxcrt.h +++ b/include/wx/wxcrt.h @@ -257,7 +257,7 @@ inline size_t wxStrlcpy(char *dest, const char *src, size_t n) { if ( n-- > len ) n = len; - wxCRT_StrncpyA(dest, src, n); + wxTmemcpy(dest, src, n); dest[n] = '\0'; } @@ -270,7 +270,7 @@ inline size_t wxStrlcpy(wchar_t *dest, const wchar_t *src, size_t n) { if ( n-- > len ) n = len; - wxCRT_StrncpyW(dest, src, n); + wxTmemcpy(dest, src, n); dest[n] = L'\0'; } From e24e9323e01ef013b54c0b38d97a8b4d15f47184 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 21 Jul 2018 16:00:08 +0200 Subject: [PATCH 5/8] Fix clobbering warnings in PNG image loading code in a better way 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 99ea06203aec0ebb1f8978cb34a50483e0fa746c ("Fix clobbering warning") and replaces it with a different solution to the same problem. --- src/common/imagpng.cpp | 151 ++++++++++++++++++++++++++--------------- 1 file changed, 95 insertions(+), 56 deletions(-) diff --git a/src/common/imagpng.cpp b/src/common/imagpng.cpp index 0644d0c82d..48aee7d5fc 100644 --- a/src/common/imagpng.cpp +++ b/src/common/imagpng.cpp @@ -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; } // ---------------------------------------------------------------------------- From e3df636d826f893d4025c2f09a3d70b38c89b129 Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Mon, 23 Jul 2018 08:33:10 +0300 Subject: [PATCH 6/8] Make DoLoadPNGFile member function of wxPNGImageData Avoid adding "data." everywhere in DoLoadPNGFile code --- src/common/imagpng.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/common/imagpng.cpp b/src/common/imagpng.cpp index 48aee7d5fc..89fee280fe 100644 --- a/src/common/imagpng.cpp +++ b/src/common/imagpng.cpp @@ -139,6 +139,8 @@ struct wxPNGImageData free( lines ); } + void DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo); + unsigned char** lines; png_uint_32 numLines; bool ok; @@ -299,11 +301,10 @@ void CopyDataFromPNG(wxImage *image, // 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 +// "returns" its result via wxPNGImageData: use its "ok" field to check // whether loading succeeded or failed. -static void -DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo, wxPNGImageData& data) +wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) { // VZ: as this function uses setjmp() the only fool-proof error handling // method is to use goto (setjmp is not really C++ dtors friendly...) @@ -362,10 +363,10 @@ DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo, wxPNGImageData& data) // 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 - if ( !data.Alloc(width, height) ) + if ( !Alloc(width, height) ) goto error; - png_read_image( png_ptr, data.lines ); + png_read_image( png_ptr, lines ); png_read_end( png_ptr, info_ptr ); #if wxUSE_PALETTE @@ -436,10 +437,10 @@ DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo, wxPNGImageData& data) // loaded successfully, now init wxImage with this data - CopyDataFromPNG(image, data.lines, width, height, color_type); + CopyDataFromPNG(image, lines, width, height, color_type); // This will indicate to the caller that loading succeeded. - data.ok = true; + ok = true; error: // Note that we only get here if both png_ptr and info_ptr are valid, @@ -458,7 +459,7 @@ wxPNGHandler::LoadFile(wxImage *image, wxinfo.stream.in = &stream; wxPNGImageData data; - DoLoadPNGFile(image, wxinfo, data); + data.DoLoadPNGFile(image, wxinfo); if ( !data.ok ) { From 58f0fd65fefacd883b3a0c93ed5157f05c05eb03 Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Mon, 23 Jul 2018 09:01:56 +0300 Subject: [PATCH 7/8] Store info_ptr and png_ptr in wxPNGImageData Avoid having to use goto in DoLoadPNGFile and have all clean-ups in wxPNGImageData destructor. --- src/common/imagpng.cpp | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/common/imagpng.cpp b/src/common/imagpng.cpp index 89fee280fe..516de8d3e6 100644 --- a/src/common/imagpng.cpp +++ b/src/common/imagpng.cpp @@ -111,6 +111,8 @@ struct wxPNGImageData { lines = NULL; numLines = 0; + info_ptr = (png_infop) NULL; + png_ptr = (png_structp) NULL; ok = false; } @@ -137,12 +139,22 @@ struct wxPNGImageData free( lines[n] ); free( lines ); + + if ( png_ptr ) + { + if ( info_ptr ) + png_destroy_read_struct( &png_ptr, &info_ptr, (png_infopp) NULL ); + else + png_destroy_read_struct( &png_ptr, (png_infopp) NULL, (png_infopp) NULL ); + } } void DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo); unsigned char** lines; png_uint_32 numLines; + png_infop info_ptr; + png_structp png_ptr; bool ok; }; @@ -306,17 +318,12 @@ void CopyDataFromPNG(wxImage *image, void wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) { - // VZ: as this function uses setjmp() the only fool-proof error handling - // method is to use goto (setjmp is not really C++ dtors friendly...) - - png_infop info_ptr = (png_infop) NULL; - png_uint_32 width, height = 0; int bit_depth, color_type, interlace_type; image->Destroy(); - png_structp png_ptr = png_create_read_struct + png_ptr = png_create_read_struct ( PNG_LIBPNG_VER_STRING, NULL, @@ -332,13 +339,10 @@ wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) info_ptr = png_create_info_struct( png_ptr ); if (!info_ptr) - { - png_destroy_read_struct( &png_ptr, (png_infopp) NULL, (png_infopp) NULL ); return; - } if (setjmp(wxinfo.jmpbuf)) - goto error; + return; png_read_info( png_ptr, info_ptr ); png_get_IHDR( png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, &interlace_type, NULL, NULL ); @@ -359,12 +363,12 @@ wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) image->Create((int)width, (int)height, (bool) false /* no need to init pixels */); if (!image->IsOk()) - goto error; + return; // 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 if ( !Alloc(width, height) ) - goto error; + return; png_read_image( png_ptr, lines ); png_read_end( png_ptr, info_ptr ); @@ -441,11 +445,6 @@ wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) // This will indicate to the caller that loading succeeded. ok = true; - -error: - // 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 ); } bool From 59942486488c62b4624f2354ef1890b794bbb3cb Mon Sep 17 00:00:00 2001 From: pavel-t <36256989+pavel-t@users.noreply.github.com> Date: Mon, 23 Jul 2018 09:21:40 +0300 Subject: [PATCH 8/8] Keep actual number of allocated lines in numLines --- src/common/imagpng.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/common/imagpng.cpp b/src/common/imagpng.cpp index 516de8d3e6..0b7c485321 100644 --- a/src/common/imagpng.cpp +++ b/src/common/imagpng.cpp @@ -118,15 +118,16 @@ struct wxPNGImageData bool Alloc(png_uint_32 width, png_uint_32 height) { - lines = (unsigned char **)calloc(height, sizeof(unsigned char *)); + lines = (unsigned char **)malloc(height * sizeof(unsigned char *)); if ( !lines ) return false; - numLines = height; - for ( unsigned int n = 0; n < numLines; n++ ) + for ( png_uint_32 n = 0; n < height; n++ ) { lines[n] = (unsigned char *)malloc( (size_t)(width * 4)); - if ( !lines[n] ) + if ( lines[n] ) + ++numLines; + else return false; } @@ -365,8 +366,6 @@ wxPNGImageData::DoLoadPNGFile(wxImage* image, wxPNGInfoStruct& wxinfo) if (!image->IsOk()) return; - // 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 if ( !Alloc(width, height) ) return;