From 5a6e8c9d65e533868a93e73492922bb39a67f50d Mon Sep 17 00:00:00 2001 From: cutealien Date: Mon, 2 Oct 2023 21:42:40 +0000 Subject: [PATCH] Merging r6522 through r6546 from branch releases/1.8 to trunk - Fixing buffer overflows in bmp and obj loaders - Fixed loading of rle4 encoded bmp images git-svn-id: svn://svn.code.sf.net/p/irrlicht/code/trunk@6547 dfc29bdd-3216-0410-991c-e03cc46cb475 --- changes.txt | 12 ++ source/Irrlicht/CImageLoaderBMP.cpp | 177 ++++++++++++++++++------- source/Irrlicht/COBJMeshFileLoader.cpp | 65 +++++---- 3 files changed, 177 insertions(+), 77 deletions(-) diff --git a/changes.txt b/changes.txt index b66657a..775c74a 100644 --- a/changes.txt +++ b/changes.txt @@ -402,6 +402,18 @@ Changes in 1.9 (not yet released) -------------------------- Changes in 1.8.6 +- COBJMeshFilerLoder: prevent buffer overruns from loading files passing negative indices. Thanks @sfan5 fore report and patch. + Patch (commit 827710f to Minetest): https://github.com/minetest/irrlicht/commit/827710f74a615f53b2a1b0c539c58c2b6124f883 +- COBJMeshFilerLoder: fix buffer overruns when loading empty face lines. Thanks @sfan5 fore report and patch. + Patch (commit 80e1609 to Minetest): https://github.com/minetest/irrlicht/commit/80e160935d3c2677344b0968c2690f63083a98dd (partially applied) +- CObjMeshFileLoader: Backport fixes from trunk to avoid unnecessary memory allocations (speedup) +- CObjMeshFileLoader: Backport fixes from trunk to avoid some buffer overruns +- CImageLoaderBMP: Fix handling 4 bit RLE encoding. Thanks @sfan5 finding first bug and reporting (had some more) +- CImageLoaderBMP: add bound checks to RLE decompression. Thanks @sfan5 for report and patch + Patch (commit 4506d23 to Minetest): https://github.com/minetest/irrlicht/commit/4506d23dc3fa48332b5ca6c05633aebdbac673be +- CImageLoaderBMP: check bitmap data against required size. Thanks @sfan5 for report and patch + Report: https://irrlicht.sourceforge.io/forum/viewtopic.php?p=307195 + Patch (commit 028cb8d to Minetest): https://github.com/minetest/irrlicht/commit/028cb8dbed8266264a5804108191f56e238db1bc - Fix OSX 10.9X build problem related to NSApplication setDelegate calls getting casted to wrong class (Bug #462 and also fixing older Bug #297) Thanks @Ryan Schmidt for bug report and patch. - Backport: Fix compiling for Apple silicon (Bugs #452 and #461). Thanks @Ryan Schmidt for bug report and updated patch. diff --git a/source/Irrlicht/CImageLoaderBMP.cpp b/source/Irrlicht/CImageLoaderBMP.cpp index d91c844..3c7b482 100644 --- a/source/Irrlicht/CImageLoaderBMP.cpp +++ b/source/Irrlicht/CImageLoaderBMP.cpp @@ -47,19 +47,35 @@ bool CImageLoaderBMP::isALoadableFileFormat(io::IReadFile* file) const } +// UB-safe overflow check +static inline bool doesOverflow(const void *base, size_t numElements, const void *end) +{ + // TODO: uintptr_t (as in original patch from sfan5) would be nicer than size_t, use once we allow c++11 + size_t baseI = reinterpret_cast(base); + size_t endI = reinterpret_cast(end); + return baseI > endI || numElements >= (endI - baseI); +} + +// check whether &p[0] to &p[_off - 1] can be accessed +#define EXIT_P_OVERFLOW(_off) if (doesOverflow(p, _off, pEnd)) goto exit +// same for d +#define EXIT_D_OVERFLOW(_off) if (doesOverflow(d, _off, destEnd)) goto exit + void CImageLoaderBMP::decompress8BitRLE(u8*& bmpData, s32 size, s32 width, s32 height, s32 pitch) const { u8* p = bmpData; + const u8* pEnd = bmpData + size; u8* newBmp = new u8[(width+pitch)*height]; u8* d = newBmp; - u8* destEnd = newBmp + (width+pitch)*height; + const u8* destEnd = newBmp + (width+pitch)*height; s32 line = 0; - while (bmpData - p < size && d < destEnd) + while (p < pEnd && d < destEnd) { if (*p == 0) { ++p; + EXIT_P_OVERFLOW(1); switch(*p) { @@ -69,37 +85,43 @@ void CImageLoaderBMP::decompress8BitRLE(u8*& bmpData, s32 size, s32 width, s32 h d = newBmp + (line*(width+pitch)); break; case 1: // end of bmp - delete [] bmpData; - bmpData = newBmp; - return; + goto exit; case 2: - ++p; d +=(u8)*p; // delta - ++p; d += ((u8)*p)*(width+pitch); + ++p; + EXIT_P_OVERFLOW(2); + d += (u8)*p; // delta + ++p; + d += ((u8)*p)*(width+pitch); ++p; break; default: { // absolute mode - s32 count = (u8)*p; ++p; - s32 readAdditional = ((2-(count%2))%2); - s32 i; + const s32 count = (u8)*p; + ++p; - for (i=0; i= width ) \ + { \ + line += row/width; \ + row %= width; \ + } \ + if ( line >= height ) \ + { \ + line = 0; /* bug anyway, this way more visible maybe */ \ + } + + void CImageLoaderBMP::decompress4BitRLE(u8*& bmpData, s32 size, s32 width, s32 height, s32 pitch) const { - s32 lineWidth = (width+1)/2+pitch; + const s32 lineWidth = (width+1)/2+pitch; u8* p = bmpData; + const u8* pEnd = bmpData + size; u8* newBmp = new u8[lineWidth*height]; - u8* d = newBmp; - u8* destEnd = newBmp + lineWidth*height; + memset(newBmp, 0, lineWidth*height); // Extra cost, but otherwise we have to code pixel skipping stuff + const u8* destEnd = newBmp + lineWidth*height; s32 line = 0; - s32 shift = 4; + s32 row = 0; - while (bmpData - p < size && d < destEnd) + while (p < pEnd) { if (*p == 0) { ++p; + EXIT_P_OVERFLOW(1); switch(*p) { case 0: // end of line ++p; ++line; - d = newBmp + (line*lineWidth); - shift = 4; + row = 0; break; case 1: // end of bmp - delete [] bmpData; - bmpData = newBmp; - return; - case 2: + goto exit; + case 2: // delta { ++p; + EXIT_P_OVERFLOW(2); s32 x = (u8)*p; ++p; s32 y = (u8)*p; ++p; - d += x/2 + y*lineWidth; - shift = x%2==0 ? 4 : 0; + row += x; + line += y; } break; default: { // absolute mode - s32 count = (u8)*p; ++p; - s32 readAdditional = ((2-((count)%2))%2); + const u32 count = (u8)*p; ++p; s32 readShift = 4; - s32 i; - for (i=0; i> readShift) & 0x0f; readShift -= 4; if (readShift < 0) { - ++*p; + ++p; readShift = 4; } @@ -177,26 +233,37 @@ void CImageLoaderBMP::decompress4BitRLE(u8*& bmpData, s32 size, s32 width, s32 h shift = 4; ++d; } - } + row += count; - for (i=0; i> 4) & 0x0f; + const s32 count = (u8)*p; // pixels to draw + ++p; + EXIT_P_OVERFLOW(1); + s32 color1 = (u8)*p; color1 = color1 & 0x0f; // lo bit (2nd,4th,... pixel) + s32 color2 = (u8)*p; color2 = (color2 >> 4) & 0x0f; // hi bits (1st,3rd,... pixel) ++p; + KEEP_ROW_LINE_INSIDE; + s32 shift = row%2 == 0 ? 4 : 0; + u8* d = newBmp + (lineWidth*line + row/2); + + EXIT_D_OVERFLOW(shiftedCount(count, shift)); for (s32 i=0; iseek(header.BitmapDataOffset); - f32 t = (header.Width) * (header.BPP / 8.0f); - s32 widthInBytes = (s32)t; - t -= widthInBytes; - if (t!=0.0f) - ++widthInBytes; - - s32 lineData = widthInBytes + ((4-(widthInBytes%4)))%4; - pitch = lineData - widthInBytes; + const s32 widthInBytes = core::ceil32(header.Width * (header.BPP / 8.0f)); + const s32 lineSize = widthInBytes + ((4-(widthInBytes%4)))%4; + pitch = lineSize - widthInBytes; u8* bmpData = new u8[header.BitmapDataSize]; file->read(bmpData, header.BitmapDataSize); @@ -296,14 +362,25 @@ IImage* CImageLoaderBMP::loadImage(io::IReadFile* file) const // decompress data if needed switch(header.Compression) { - case 1: // 8 bit rle + case 1: // 8 bit RLE decompress8BitRLE(bmpData, header.BitmapDataSize, header.Width, header.Height, pitch); + header.BitmapDataSize = (header.Width + pitch) * header.Height; break; - case 2: // 4 bit rle + case 2: // 4 bit RLE decompress4BitRLE(bmpData, header.BitmapDataSize, header.Width, header.Height, pitch); + header.BitmapDataSize = ((header.Width+1)/2 + pitch) * header.Height; break; } + if (header.BitmapDataSize < lineSize * header.Height) + { + os::Printer::log("Bitmap data is cut off.", ELL_ERROR); + + delete [] paletteData; + delete [] bmpData; + return 0; + } + // create surface // no default constructor from packed area! ARM problem! diff --git a/source/Irrlicht/COBJMeshFileLoader.cpp b/source/Irrlicht/COBJMeshFileLoader.cpp index 70a46ae..a9ed304 100644 --- a/source/Irrlicht/COBJMeshFileLoader.cpp +++ b/source/Irrlicht/COBJMeshFileLoader.cpp @@ -67,13 +67,25 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file) { if (!file) return 0; + long filesize = file->getSize(); + if (!filesize) + return 0; + + const io::path fullName = file->getFileName(); + const io::path relPath = FileSystem->getFileDir(fullName)+"/"; + + c8* buf = new c8[filesize]; + filesize = file->read((void*)buf, filesize); + if ( filesize <= 0 ) + { + delete[] buf; + return 0; + } if ( getMeshTextureLoader() ) getMeshTextureLoader()->setMeshFile(file); - const long filesize = file->getSize(); - if (!filesize) - return 0; + const c8* const bufEnd = buf+filesize; const u32 WORD_BUFFER_LENGTH = 512; @@ -85,14 +97,6 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file) Materials.push_back(currMtl); u32 smoothingGroup=0; - const io::path fullName = file->getFileName(); - const io::path relPath = FileSystem->getFileDir(fullName)+"/"; - - c8* buf = new c8[filesize]; - memset(buf, 0, filesize); - file->read((void*)buf, filesize); - const c8* const bufEnd = buf+filesize; - // Process obj information const c8* bufPtr = buf; core::stringc grpName, mtlName; @@ -245,7 +249,7 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file) u32 wlength = copyWord(vertexWord, linePtr, WORD_BUFFER_LENGTH, endPtr); // this function will also convert obj's 1-based index to c++'s 0-based index retrieveVertexIndices(vertexWord, Idx, vertexWord+wlength+1, vertexBuffer.size(), textureCoordBuffer.size(), normalsBuffer.size()); - if ( -1 != Idx[0] && Idx[0] < (irr::s32)vertexBuffer.size() ) + if ( Idx[0] >= 0 && Idx[0] < (irr::s32)vertexBuffer.size() ) v.Pos = vertexBuffer[Idx[0]]; else { @@ -253,11 +257,11 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file) delete [] buf; return 0; } - if ( -1 != Idx[1] && Idx[1] < (irr::s32)textureCoordBuffer.size() ) + if ( Idx[1] >= 0 && Idx[1] < (irr::s32)textureCoordBuffer.size() ) v.TCoords = textureCoordBuffer[Idx[1]]; else v.TCoords.set(0.0f,0.0f); - if ( -1 != Idx[2] && Idx[2] < (irr::s32)normalsBuffer.size() ) + if ( Idx[2] >= 0 && Idx[2] < (irr::s32)normalsBuffer.size() ) v.Normal = normalsBuffer[Idx[2]]; else { @@ -285,23 +289,30 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file) } // triangulate the face - const int c = faceCorners[0]; - for ( u32 i = 1; i < faceCorners.size() - 1; ++i ) + if ( faceCorners.size() >= 3) { - // Add a triangle - const int a = faceCorners[i + 1]; - const int b = faceCorners[i]; - if (a != b && a != c && b != c) // ignore degenerated faces. We can get them when we merge vertices above in the VertMap. + const int c = faceCorners[0]; + for ( u32 i = 1; i < faceCorners.size() - 1; ++i ) { - mbIndexBuffer.push_back(a); - mbIndexBuffer.push_back(b); - mbIndexBuffer.push_back(c); - } - else - { - ++degeneratedFaces; + // Add a triangle + const int a = faceCorners[i + 1]; + const int b = faceCorners[i]; + if (a != b && a != c && b != c) // ignore degenerated faces. We can get them when we merge vertices above in the VertMap. + { + mbIndexBuffer.push_back(a); + mbIndexBuffer.push_back(b); + mbIndexBuffer.push_back(c); + } + else + { + ++degeneratedFaces; + } } } + else + { + os::Printer::log("Too few vertices in this line", wordBuffer.c_str()); + } } break;