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

@@ -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.

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<size_t>(base);
+	size_t endI = reinterpret_cast<size_t>(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;
+				goto exit;
-				bmpData = newBmp;
-				return;
 			case 2:
-				++p; d +=(u8)*p;  // delta
+				++p;
-				++p; d += ((u8)*p)*(width+pitch);
+				EXIT_P_OVERFLOW(2);
+				d += (u8)*p; // delta
+				++p;
+				d += ((u8)*p)*(width+pitch);
 				++p;
 				break;
 			default:
 				{
 					// absolute mode
-					s32 count = (u8)*p; ++p;
+					const s32 count = (u8)*p;
-					s32 readAdditional = ((2-(count%2))%2);
+					++p;
-					s32 i;
 
-					for (i=0; i<count; ++i)
+					EXIT_P_OVERFLOW(count);
+					EXIT_D_OVERFLOW(count);
+					for (s32 i=0; i<count; ++i)
 					{
 						*d = *p;
 						++p;
 						++d;
 					}
 
-					for (i=0; i<readAdditional; ++i)
+					const s32 readAdditional = ((2-(count%2))%2);
+					EXIT_P_OVERFLOW(readAdditional);
+					for (s32 i=0; i<readAdditional; ++i)
 						++p;
 				}
 			}
 		}
 		else
 		{
-			s32 count = (u8)*p; ++p;
+			const s32 count = (u8)*p; ++p;
+			EXIT_P_OVERFLOW(1);
 			u8 color = *p; ++p;
+			EXIT_D_OVERFLOW(count);
 			for (s32 i=0; i<count; ++i)
 			{
 				*d = color;
@@ -108,63 +130,97 @@ void CImageLoaderBMP::decompress8BitRLE(u8*& bmpData, s32 size, s32 width, s32 h
 		}
 	}
 
+exit:
 	delete [] bmpData;
 	bmpData = newBmp;
 }
 
 
+// how many new bytes will be touched given the current state of decompress4BitRLE
+static inline u32 shiftedCount(s32 count, s32 shift)
+{
+	IRR_DEBUG_BREAK_IF(count <= 0)
+
+	u32 ret = 0;
+	if ( shift == 0 )	// using up half of an old byte
+	{
+		--count;
+	}
+	ret += (count / 2) + (count % 2);
+	return ret;
+}
+
+// Ensure current row/line are inside width/height
+// Didn't find any documentation how BMP's are supposed to handle this
+// I think in general "good" bmp's are supposed to not go over line-ends
+#define KEEP_ROW_LINE_INSIDE \
+	if ( row >= 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;
+	memset(newBmp, 0, lineWidth*height);	// Extra cost, but otherwise we have to code pixel skipping stuff
-	u8* destEnd = newBmp + lineWidth*height;
+	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);
+				row = 0;
-				shift = 4;
 				break;
 			case 1: // end of bmp
-				delete [] bmpData;
+				goto exit;
-				bmpData = newBmp;
+			case 2: // delta
-				return;
-			case 2:
 				{
 					++p;
+					EXIT_P_OVERFLOW(2);
 					s32 x = (u8)*p; ++p;
 					s32 y = (u8)*p; ++p;
-					d += x/2 + y*lineWidth;
+					row += x;
-					shift = x%2==0 ? 4 : 0;
+					line += y;
 				}
 				break;
 			default:
 				{
 					// absolute mode
-					s32 count = (u8)*p; ++p;
+					const u32 count = (u8)*p; ++p;
-					s32 readAdditional = ((2-((count)%2))%2);
 					s32 readShift = 4;
-					s32 i;
 
-					for (i=0; i<count; ++i)
+					KEEP_ROW_LINE_INSIDE;
+					s32 shift = row%2 == 0 ? 4 : 0;
+					u8* d = newBmp + (lineWidth*line + row/2);
+
+					EXIT_P_OVERFLOW(shiftedCount(count, readShift));
+					EXIT_D_OVERFLOW(shiftedCount(count, shift));
+					for (u32 i=0; i<count; ++i)
 					{
 						s32 color = (((u8)*p) >> 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<readAdditional; ++i)
+					// pixels always come in 2-byte packages with unused half-bytes filled with zeroes
+					const u32 modCount = (count%4);
+					const u32 readAdditional = modCount == 1 ? 2 : (modCount==0?0:1); // jump 2,1,1,0 for count 1,2,3,4
+					EXIT_P_OVERFLOW(readAdditional);
+					for (u32 i=0; i<readAdditional; ++i)
 						++p;
 				}
 			}
 		}
-		else
+		else // encoded mode
 		{
-			s32 count = (u8)*p; ++p;
+			const s32 count = (u8)*p;	// pixels to draw
-			s32 color1 = (u8)*p; color1 = color1 & 0x0f;
+			++p;
-			s32 color2 = (u8)*p; color2 = (color2 >> 4) & 0x0f;
+			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; i<count; ++i)
 			{
 				u8 mask = 0x0f << shift;
-				u8 toSet = (shift==0 ? color1 : color2) << shift;
+				u8 toSet = ((i%2==0) ? color2 : color1) << shift;
-				*d = (*d & (~mask)) | (toSet & mask);
+				*d = (*d & (~mask)) | toSet;
 
 				shift -= 4;
 				if (shift < 0)
@@ -205,14 +272,18 @@ void CImageLoaderBMP::decompress4BitRLE(u8*& bmpData, s32 size, s32 width, s32 h
 					++d;
 				}
 			}
+			row += count;
 		}
 	}
 
+exit:
 	delete [] bmpData;
 	bmpData = newBmp;
 }
 
-
+#undef EXIT_P_OVERFLOW
+#undef EXIT_D_OVERFLOW
+#undef KEEP_ROW_LINE_INSIDE
 
 //! creates a surface from the file
 IImage* CImageLoaderBMP::loadImage(io::IReadFile* file) const
@@ -281,14 +352,9 @@ IImage* CImageLoaderBMP::loadImage(io::IReadFile* file) const
 
 	file->seek(header.BitmapDataOffset);
 
-	f32 t = (header.Width) * (header.BPP / 8.0f);
+	const s32 widthInBytes = core::ceil32(header.Width * (header.BPP / 8.0f));
-	s32 widthInBytes = (s32)t;
+	const s32 lineSize = widthInBytes + ((4-(widthInBytes%4)))%4;
-	t -= widthInBytes;
+	pitch = lineSize - widthInBytes;
-	if (t!=0.0f)
-		++widthInBytes;
-
-	s32 lineData = widthInBytes + ((4-(widthInBytes%4)))%4;
-	pitch = lineData - 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!

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();
+	const c8* const bufEnd = buf+filesize;
-	if (!filesize)
-		return 0;
 
 	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];
+			if ( faceCorners.size() >= 3)
-			for ( u32 i = 1; i < faceCorners.size() - 1; ++i )
 			{
-				// Add a triangle
+				const int c = faceCorners[0];
-				const int a = faceCorners[i + 1];
+				for ( u32 i = 1; i < faceCorners.size() - 1; ++i )
-				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);
+					// Add a triangle
-					mbIndexBuffer.push_back(b);
+					const int a = faceCorners[i + 1];
-					mbIndexBuffer.push_back(c);
+					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.
-				else
+					{
-				{
+						mbIndexBuffer.push_back(a);
-					++degeneratedFaces;
+						mbIndexBuffer.push_back(b);
+						mbIndexBuffer.push_back(c);
+					}
+					else
+					{
+						++degeneratedFaces;
+					}
 				}
 			}
+			else
+			{
+				os::Printer::log("Too few vertices in this line", wordBuffer.c_str());
+			}
 		}
 		break;