From a45b04ffb4c0583ef3c8727ea0f73d40e3662e9d Mon Sep 17 00:00:00 2001 From: HybridDog <3192173+HybridDog@users.noreply.github.com> Date: Tue, 3 Dec 2024 16:52:48 +0100 Subject: [PATCH] Less explicit memory management in Irrlicht image writer classes (#15493) CImageWriterPNG::writeImage() and writeJPEGFile() explicitly allocate and deallocate memory with `new` and `delete`, which is prone to programming errors. The code also has non-functional error handling: When memory allocation fails, `new` throws an `std::bad_alloc` exception and never returns `nullptr`, so the check for `nullptr` is always false. Co-authored-by: DS --- irr/src/CImageWriterJPG.cpp | 33 ++++++++++++-------------- irr/src/CImageWriterPNG.cpp | 47 +++++++++++++++---------------------- 2 files changed, 34 insertions(+), 46 deletions(-) diff --git a/irr/src/CImageWriterJPG.cpp b/irr/src/CImageWriterJPG.cpp index fa8ad64bc..1220c47e6 100644 --- a/irr/src/CImageWriterJPG.cpp +++ b/irr/src/CImageWriterJPG.cpp @@ -10,6 +10,7 @@ #include "os.h" #include // IWYU pragma: keep (required for jpeglib.h) +#include #include #include @@ -130,32 +131,28 @@ static bool writeJPEGFile(io::IWriteFile *file, IImage *image, u32 quality) jpeg_set_quality(&cinfo, quality, TRUE); jpeg_start_compress(&cinfo, TRUE); - u8 *dest = new u8[dim.Width * 3]; + std::unique_ptr dest{new u8[dim.Width * 3]}; - if (dest) { - const u32 pitch = image->getPitch(); - JSAMPROW row_pointer[1]; /* pointer to JSAMPLE row[s] */ - row_pointer[0] = dest; + const u32 pitch = image->getPitch(); + JSAMPROW row_pointer[1]; /* pointer to JSAMPLE row[s] */ + row_pointer[0] = dest.get(); - u8 *src = (u8 *)image->getData(); + u8 *src = (u8 *)image->getData(); - while (cinfo.next_scanline < cinfo.image_height) { - // convert next line - format(src, dim.Width, dest); - src += pitch; - jpeg_write_scanlines(&cinfo, row_pointer, 1); - } - - delete[] dest; - - /* Step 6: Finish compression */ - jpeg_finish_compress(&cinfo); + while (cinfo.next_scanline < cinfo.image_height) { + // convert next line + format(src, dim.Width, dest.get()); + src += pitch; + jpeg_write_scanlines(&cinfo, row_pointer, 1); } + /* Step 6: Finish compression */ + jpeg_finish_compress(&cinfo); + /* Step 7: Destroy */ jpeg_destroy_compress(&cinfo); - return (dest != 0); + return true; } } // namespace video diff --git a/irr/src/CImageWriterPNG.cpp b/irr/src/CImageWriterPNG.cpp index 14c9f2d9c..35e33c3d5 100644 --- a/irr/src/CImageWriterPNG.cpp +++ b/irr/src/CImageWriterPNG.cpp @@ -10,6 +10,7 @@ #include "os.h" // for logging #include // use system lib png +#include namespace irr { @@ -94,22 +95,23 @@ bool CImageWriterPNG::writeImage(io::IWriteFile *file, IImage *image, u32 param) png_set_write_fn(png_ptr, file, user_write_data_fcn, NULL); // Set info + core::dimension2d img_dim = image->getDimension(); switch (image->getColorFormat()) { case ECF_A8R8G8B8: case ECF_A1R5G5B5: png_set_IHDR(png_ptr, info_ptr, - image->getDimension().Width, image->getDimension().Height, + img_dim.Width, img_dim.Height, 8, PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT); break; default: png_set_IHDR(png_ptr, info_ptr, - image->getDimension().Width, image->getDimension().Height, + img_dim.Width, img_dim.Height, 8, PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT); } - s32 lineWidth = image->getDimension().Width; + s32 lineWidth = img_dim.Width; switch (image->getColorFormat()) { case ECF_R8G8B8: case ECF_R5G6B5: @@ -123,61 +125,52 @@ bool CImageWriterPNG::writeImage(io::IWriteFile *file, IImage *image, u32 param) default: break; } - u8 *tmpImage = new u8[image->getDimension().Height * lineWidth]; - if (!tmpImage) { - os::Printer::log("PNGWriter: Internal PNG create image failure", file->getFileName(), ELL_ERROR); - png_destroy_write_struct(&png_ptr, &info_ptr); - return false; - } + std::unique_ptr tmpImage{new u8[img_dim.Height * lineWidth]}; + auto num_pixels = img_dim.Height * img_dim.Width; u8 *data = (u8 *)image->getData(); switch (image->getColorFormat()) { case ECF_R8G8B8: - CColorConverter::convert_R8G8B8toR8G8B8(data, image->getDimension().Height * image->getDimension().Width, tmpImage); + CColorConverter::convert_R8G8B8toR8G8B8(data, num_pixels, + tmpImage.get()); break; case ECF_A8R8G8B8: - CColorConverter::convert_A8R8G8B8toA8R8G8B8(data, image->getDimension().Height * image->getDimension().Width, tmpImage); + CColorConverter::convert_A8R8G8B8toA8R8G8B8(data, num_pixels, + tmpImage.get()); break; case ECF_R5G6B5: - CColorConverter::convert_R5G6B5toR8G8B8(data, image->getDimension().Height * image->getDimension().Width, tmpImage); + CColorConverter::convert_R5G6B5toR8G8B8(data, num_pixels, + tmpImage.get()); break; case ECF_A1R5G5B5: - CColorConverter::convert_A1R5G5B5toA8R8G8B8(data, image->getDimension().Height * image->getDimension().Width, tmpImage); + CColorConverter::convert_A1R5G5B5toA8R8G8B8(data, num_pixels, + tmpImage.get()); break; // TODO: Error handling in case of unsupported color format default: os::Printer::log("CImageWriterPNG does not support image format", ColorFormatNames[image->getColorFormat()], ELL_WARNING); png_destroy_write_struct(&png_ptr, &info_ptr); - delete[] tmpImage; return false; } // Create array of pointers to rows in image data // Used to point to image rows - u8 **RowPointers = new png_bytep[image->getDimension().Height]; - if (!RowPointers) { - os::Printer::log("PNGWriter: Internal PNG create row pointers failure", file->getFileName(), ELL_ERROR); - png_destroy_write_struct(&png_ptr, &info_ptr); - delete[] tmpImage; - return false; - } + std::unique_ptr RowPointers{new u8*[img_dim.Height]}; - data = tmpImage; + data = tmpImage.get(); // Fill array of pointers to rows in image data - for (u32 i = 0; i < image->getDimension().Height; ++i) { + for (u32 i = 0; i < img_dim.Height; ++i) { RowPointers[i] = data; data += lineWidth; } // for proper error handling if (setjmp(png_jmpbuf(png_ptr))) { png_destroy_write_struct(&png_ptr, &info_ptr); - delete[] RowPointers; - delete[] tmpImage; return false; } - png_set_rows(png_ptr, info_ptr, RowPointers); + png_set_rows(png_ptr, info_ptr, RowPointers.get()); if (image->getColorFormat() == ECF_A8R8G8B8 || image->getColorFormat() == ECF_A1R5G5B5) png_write_png(png_ptr, info_ptr, PNG_TRANSFORM_BGR, NULL); @@ -185,8 +178,6 @@ bool CImageWriterPNG::writeImage(io::IWriteFile *file, IImage *image, u32 param) png_write_png(png_ptr, info_ptr, PNG_TRANSFORM_IDENTITY, NULL); } - delete[] RowPointers; - delete[] tmpImage; png_destroy_write_struct(&png_ptr, &info_ptr); return true; }