From 0538e64b13c699387f73a7c4fc2fcecd71832371 Mon Sep 17 00:00:00 2001 From: Vincent Rabaud Date: Fri, 3 Jan 2025 12:13:18 +0100 Subject: [PATCH] Fix leaks in cv:Merge pull request #26701 from vrabaud:png_leak Fix leaks in cv::PngDecoder #26701 Bug: oss-fuzz:386688709 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [ ] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake --- modules/imgcodecs/src/grfmt_png.cpp | 75 +++++++++++++---------------- modules/imgcodecs/src/grfmt_png.hpp | 10 ++-- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/modules/imgcodecs/src/grfmt_png.cpp b/modules/imgcodecs/src/grfmt_png.cpp index dedc9be515..3236abc637 100644 --- a/modules/imgcodecs/src/grfmt_png.cpp +++ b/modules/imgcodecs/src/grfmt_png.cpp @@ -141,7 +141,6 @@ APNGFrame::APNGFrame() _transparencySize = 0; _delayNum = 1; _delayDen = 1000; - _rows = NULL; } APNGFrame::~APNGFrame() {} @@ -159,7 +158,7 @@ bool APNGFrame::setMat(const cv::Mat& src, unsigned delayNum, unsigned delayDen) _height = src.rows; _colorType = src.channels() == 1 ? PNG_COLOR_TYPE_GRAY : src.channels() == 3 ? PNG_COLOR_TYPE_RGB : PNG_COLOR_TYPE_RGB_ALPHA; _pixels = src.data; - _rows = new png_bytep[_height * sizeof(png_bytep)]; + _rows.resize(_height); for (unsigned int i = 0; i < _height; ++i) _rows[i] = _pixels + i * rowbytes; @@ -178,7 +177,6 @@ void APNGFrame::setTransparencySize(int transparencySize) { _transparencySize = void APNGFrame::setDelayNum(unsigned int delayNum) { _delayNum = delayNum; } void APNGFrame::setDelayDen(unsigned int delayDen) { _delayDen = delayDen; } void APNGFrame::setPixels(unsigned char* pixels) { _pixels = pixels; } -void APNGFrame::setRows(unsigned char** rows) { _rows = rows; } PngDecoder::PngDecoder() { @@ -282,7 +280,7 @@ bool PngDecoder::readHeader() id = read_chunk(m_chunkIHDR); } - if (!(id == id_IHDR && m_chunkIHDR.size == 25)) + if (!(id == id_IHDR && m_chunkIHDR.p.size() == 25)) return false; while (true) @@ -302,37 +300,37 @@ bool PngDecoder::readHeader() break; } - if (id == id_acTL && chunk.size == 20) + if (id == id_acTL && chunk.p.size() == 20) { - m_animation.loop_count = png_get_uint_32(chunk.p + 12); + m_animation.loop_count = png_get_uint_32(&chunk.p[12]); if (chunk.p[8] > 0) { chunk.p[8] = 0; chunk.p[9] = 0; - m_frame_count = png_get_uint_32(chunk.p + 8); + m_frame_count = png_get_uint_32(&chunk.p[8]); m_frame_count++; } else - m_frame_count = png_get_uint_32(chunk.p + 8); + m_frame_count = png_get_uint_32(&chunk.p[8]); } if (id == id_fcTL) { m_is_fcTL_loaded = true; - w0 = png_get_uint_32(chunk.p + 12); - h0 = png_get_uint_32(chunk.p + 16); - x0 = png_get_uint_32(chunk.p + 20); - y0 = png_get_uint_32(chunk.p + 24); - delay_num = png_get_uint_16(chunk.p + 28); - delay_den = png_get_uint_16(chunk.p + 30); + w0 = png_get_uint_32(&chunk.p[12]); + h0 = png_get_uint_32(&chunk.p[16]); + x0 = png_get_uint_32(&chunk.p[20]); + y0 = png_get_uint_32(&chunk.p[24]); + delay_num = png_get_uint_16(&chunk.p[28]); + delay_den = png_get_uint_16(&chunk.p[30]); dop = chunk.p[32]; bop = chunk.p[33]; } if (id == id_bKGD) { - int bgcolor = png_get_uint_32(chunk.p + 8); + int bgcolor = png_get_uint_32(&chunk.p[8]); m_animation.bgcolor[3] = (bgcolor >> 24) & 0xFF; m_animation.bgcolor[2] = (bgcolor >> 16) & 0xFF; m_animation.bgcolor[1] = (bgcolor >> 8) & 0xFF; @@ -461,39 +459,37 @@ bool PngDecoder::readData( Mat& img ) } else { - delete[] chunk.p; return false; } - w0 = png_get_uint_32(chunk.p + 12); - h0 = png_get_uint_32(chunk.p + 16); - x0 = png_get_uint_32(chunk.p + 20); - y0 = png_get_uint_32(chunk.p + 24); - delay_num = png_get_uint_16(chunk.p + 28); - delay_den = png_get_uint_16(chunk.p + 30); + w0 = png_get_uint_32(&chunk.p[12]); + h0 = png_get_uint_32(&chunk.p[16]); + x0 = png_get_uint_32(&chunk.p[20]); + y0 = png_get_uint_32(&chunk.p[24]); + delay_num = png_get_uint_16(&chunk.p[28]); + delay_den = png_get_uint_16(&chunk.p[30]); dop = chunk.p[32]; bop = chunk.p[33]; if (int(x0 + w0) > img.cols || int(y0 + h0) > img.rows || dop > 2 || bop > 1) { - delete[] chunk.p; return false; } - memcpy(m_chunkIHDR.p + 8, chunk.p + 12, 8); + memcpy(&m_chunkIHDR.p[8], &chunk.p[12], 8); return true; } else if (id == id_IDAT) { m_is_IDAT_loaded = true; - png_process_data(png_ptr, info_ptr, chunk.p, chunk.size); + png_process_data(png_ptr, info_ptr, chunk.p.data(), chunk.p.size()); } else if (id == id_fdAT && m_is_fcTL_loaded) { m_is_IDAT_loaded = true; - png_save_uint_32(chunk.p + 4, chunk.size - 16); - memcpy(chunk.p + 8, "IDAT", 4); - png_process_data(png_ptr, info_ptr, chunk.p + 4, chunk.size - 4); + png_save_uint_32(&chunk.p[4], static_cast(chunk.p.size() - 16)); + memcpy(&chunk.p[8], "IDAT", 4); + png_process_data(png_ptr, info_ptr, &chunk.p[4], chunk.p.size() - 4); } else if (id == id_IEND) { @@ -514,13 +510,10 @@ bool PngDecoder::readData( Mat& img ) else return false; - delete[] chunk.p; return true; } else - png_process_data(png_ptr, info_ptr, chunk.p, chunk.size); - - delete[] chunk.p; + png_process_data(png_ptr, info_ptr, chunk.p.data(), chunk.p.size()); } return false; } @@ -613,7 +606,7 @@ bool PngDecoder::nextPage() { return ++m_frame_no < (int)m_frame_count; } -void PngDecoder::compose_frame(unsigned char** rows_dst, unsigned char** rows_src, unsigned char _bop, uint32_t x, uint32_t y, uint32_t w, uint32_t h, int channels) +void PngDecoder::compose_frame(std::vector& rows_dst, const std::vector& rows_src, unsigned char _bop, uint32_t x, uint32_t y, uint32_t w, uint32_t h, int channels) { uint32_t i, j; int u, v, al; @@ -668,15 +661,15 @@ uint32_t PngDecoder::read_chunk(Chunk& chunk) unsigned char len[4]; if (read_from_io(&len, 4, 1) == 1) { - chunk.size = png_get_uint_32(len) + 12; - if (chunk.size > PNG_USER_CHUNK_MALLOC_MAX) + const size_t size = png_get_uint_32(len) + 12; + if (size > PNG_USER_CHUNK_MALLOC_MAX) { CV_LOG_WARNING(NULL, "chunk data is too large"); } - chunk.p = new unsigned char[chunk.size]; - memcpy(chunk.p, len, 4); - if (read_from_io(chunk.p + 4, chunk.size - 4, 1) == 1) - return *(uint32_t*)(chunk.p + 4); + chunk.p.resize(size); + memcpy(chunk.p.data(), len, 4); + if (read_from_io(&chunk.p[4], chunk.p.size() - 4, 1) == 1) + return *(uint32_t*)(&chunk.p[4]); } return 0; } @@ -709,7 +702,7 @@ bool PngDecoder::processing_start(void* frame_ptr, const Mat& img) png_set_tRNS_to_alpha(png_ptr); png_process_data(png_ptr, info_ptr, header, 8); - png_process_data(png_ptr, info_ptr, m_chunkIHDR.p, m_chunkIHDR.size); + png_process_data(png_ptr, info_ptr, m_chunkIHDR.p.data(), m_chunkIHDR.p.size()); if ((m_color_type & PNG_COLOR_MASK_COLOR) && img.channels() > 1 && !m_use_rgb) png_set_bgr(png_ptr); // convert RGB to BGR @@ -719,7 +712,7 @@ bool PngDecoder::processing_start(void* frame_ptr, const Mat& img) png_set_rgb_to_gray(png_ptr, 1, 0.299, 0.587); // RGB->Gray for (size_t i = 0; i < m_chunksInfo.size(); i++) - png_process_data(png_ptr, info_ptr, m_chunksInfo[i].p, m_chunksInfo[i].size); + png_process_data(png_ptr, info_ptr, m_chunksInfo[i].p.data(), m_chunksInfo[i].p.size()); return true; } diff --git a/modules/imgcodecs/src/grfmt_png.hpp b/modules/imgcodecs/src/grfmt_png.hpp index c23fb8ab54..4ba22d69d7 100644 --- a/modules/imgcodecs/src/grfmt_png.hpp +++ b/modules/imgcodecs/src/grfmt_png.hpp @@ -49,11 +49,12 @@ #include "bitstrm.hpp" #include #include +#include namespace cv { -struct Chunk { unsigned char* p; uint32_t size; }; +struct Chunk { std::vector p; }; struct OP { unsigned char* p; uint32_t size; int x, y, w, h, valid, filters; }; typedef struct { @@ -101,8 +102,7 @@ public: unsigned int getDelayDen() const { return _delayDen; } void setDelayDen(unsigned int delayDen); - unsigned char** getRows() const { return _rows; } - void setRows(unsigned char** rows); + std::vector& getRows() { return _rows; } private: unsigned char* _pixels; @@ -115,7 +115,7 @@ private: int _transparencySize; unsigned int _delayNum; unsigned int _delayDen; - unsigned char** _rows; + std::vector _rows; }; class PngDecoder CV_FINAL : public BaseImageDecoder @@ -136,7 +136,7 @@ protected: static void row_fn(png_structp png_ptr, png_bytep new_row, png_uint_32 row_num, int pass); bool processing_start(void* frame_ptr, const Mat& img); bool processing_finish(); - void compose_frame(unsigned char** rows_dst, unsigned char** rows_src, unsigned char bop, uint32_t x, uint32_t y, uint32_t w, uint32_t h, int channels); + void compose_frame(std::vector& rows_dst, const std::vector& rows_src, unsigned char bop, uint32_t x, uint32_t y, uint32_t w, uint32_t h, int channels); size_t read_from_io(void* _Buffer, size_t _ElementSize, size_t _ElementCount); uint32_t read_chunk(Chunk& chunk);