(svn r22886) [1.1] -Backport from trunk:

- Fix: Harden memory allocation (r22881, r22880, r22875)
- Fix: Validate image dimensions before loading [FS#4747] (r22878, r22877, r22874, r22873)
- Fix: Perform stricter checks on RLE compressed BMP images [FS#4746] (r22872, r22871)

src/bmp.cpp

@@ -130,53 +130,63 @@ static inline bool BmpRead4(BmpBuffer *buffer, BmpInfo *info, BmpData *data)
  */
 static inline bool BmpRead4Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data)
 {
-	uint i;
 	uint x = 0;
 	uint y = info->height - 1;
-	byte n, c, b;
 	byte *pixel = &data->bitmap[y * info->width];
 	while (y != 0 || x < info->width) {
 		if (EndOfBuffer(buffer)) return false; // the file is shorter than expected
-		n = ReadByte(buffer);
+
-		c = ReadByte(buffer);
+		byte n = ReadByte(buffer);
+		byte c = ReadByte(buffer);
 		if (n == 0) {
 			switch (c) {
-			case 0: // end of line
+				case 0: // end of line
-				x = 0;
+					x = 0;
-				pixel = &data->bitmap[--y * info->width];
+					if (y == 0) return false;
-				break;
+					pixel = &data->bitmap[--y * info->width];
-			case 1: // end of bitmap
+					break;
-				x = info->width;
+
-				y = 0;
+				case 1: // end of bitmap
-				pixel = NULL;
+					return true;
-				break;
+
-			case 2: // delta
+				case 2: { // delta
-				x += ReadByte(buffer);
+					if (EndOfBuffer(buffer)) return false;
-				i = ReadByte(buffer);
+					byte dx = ReadByte(buffer);
-				if (x >= info->width || (y == 0 && i > 0)) return false;
+					byte dy = ReadByte(buffer);
-				y -= i;
+
-				pixel = &data->bitmap[y * info->width + x];
+					/* Check for over- and underflow. */
-				break;
+					if (x + dx >= info->width || x + dx < x || dy > y) return false;
-			default: // uncompressed
+
-				i = 0;
+					x += dx;
-				while (i++ < c) {
+					y -= dy;
-					if (EndOfBuffer(buffer) || x >= info->width) return false;
+					pixel = &data->bitmap[y * info->width + x];
-					b = ReadByte(buffer);
+					break;
-					*pixel++ = GB(b, 4, 4);
+				}
-					x++;
+
-					if (x < info->width && i++ < c) {
+				default: { // uncompressed
-						*pixel++ = GB(b, 0, 4);
+					uint i = 0;
-						x++;
+					while (i++ < c) {
-					}
+						if (EndOfBuffer(buffer) || x >= info->width) return false;
+						byte b = ReadByte(buffer);
+						*pixel++ = GB(b, 4, 4);
+						x++;
+						if (i++ < c) {
+							if (x >= info->width) return false;
+							*pixel++ = GB(b, 0, 4);
+							x++;
+						}
+					}
+					/* Padding for 16 bit align */
+					SkipBytes(buffer, ((c + 1) / 2) % 2);
+					break;
 				}
-				/* Padding for 16 bit align */
-				SkipBytes(buffer, ((c + 1) / 2) % 2);
-				break;
 			}
 		} else {
-			i = 0;
+			/* Apparently it is common to encounter BMPs where the count of
-			while (i++ < n) {
+			 * pixels to be written is higher than the remaining line width.
-				if (EndOfBuffer(buffer) || x >= info->width) return false;
+			 * Ignore the superfluous pixels instead of reporting an error. */
+			uint i = 0;
+			while (x < info->width && i++ < n) {
 				*pixel++ = GB(c, 4, 4);
 				x++;
 				if (x < info->width && i++ < n) {
@@ -213,43 +223,55 @@ static inline bool BmpRead8(BmpBuffer *buffer, BmpInfo *info, BmpData *data)
  */
 static inline bool BmpRead8Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data)
 {
-	uint i;
 	uint x = 0;
 	uint y = info->height - 1;
-	byte n, c;
 	byte *pixel = &data->bitmap[y * info->width];
 	while (y != 0 || x < info->width) {
 		if (EndOfBuffer(buffer)) return false; // the file is shorter than expected
-		n = ReadByte(buffer);
+
-		c = ReadByte(buffer);
+		byte n = ReadByte(buffer);
+		byte c = ReadByte(buffer);
 		if (n == 0) {
 			switch (c) {
-			case 0: // end of line
+				case 0: // end of line
-				x = 0;
+					x = 0;
-				pixel = &data->bitmap[--y * info->width];
+					if (y == 0) return false;
-				break;
+					pixel = &data->bitmap[--y * info->width];
-			case 1: // end of bitmap
+					break;
-				x = info->width;
+
-				y = 0;
+				case 1: // end of bitmap
-				pixel = NULL;
+					return true;
-				break;
+
-			case 2: // delta
+				case 2: { // delta
-				x += ReadByte(buffer);
+					if (EndOfBuffer(buffer)) return false;
-				i = ReadByte(buffer);
+					byte dx = ReadByte(buffer);
-				if (x >= info->width || (y == 0 && i > 0)) return false;
+					byte dy = ReadByte(buffer);
-				y -= i;
+
-				pixel = &data->bitmap[y * info->width + x];
+					/* Check for over- and underflow. */
-				break;
+					if (x + dx >= info->width || x + dx < x || dy > y) return false;
-			default: // uncompressed
+
-				if ((x += c) > info->width) return false;
+					x += dx;
-				for (i = 0; i < c; i++) *pixel++ = ReadByte(buffer);
+					y -= dy;
-				/* Padding for 16 bit align */
+					pixel = &data->bitmap[y * info->width + x];
-				SkipBytes(buffer, c % 2);
+					break;
-				break;
+				}
+
+				default: { // uncompressed
+					for (uint i = 0; i < c; i++) {
+						if (EndOfBuffer(buffer) || x >= info->width) return false;
+						*pixel++ = ReadByte(buffer);
+						x++;
+					}
+					/* Padding for 16 bit align */
+					SkipBytes(buffer, c % 2);
+					break;
+				}
 			}
 		} else {
-			for (i = 0; i < n; i++) {
+			/* Apparently it is common to encounter BMPs where the count of
-				if (x >= info->width) return false;
+			 * pixels to be written is higher than the remaining line width.
+			 * Ignore the superfluous pixels instead of reporting an error. */
+			for (uint i = 0; x < info->width && i < n; i++) {
 				*pixel++ = c;
 				x++;
 			}

src/core/alloc_func.hpp

@@ -22,6 +22,29 @@
 void NORETURN MallocError(size_t size);
 void NORETURN ReallocError(size_t size);
 
+/**
+ * Checks whether allocating memory would overflow size_t.
+ *
+ * @param element_size Size of the structure to allocate.
+ * @param num_elements Number of elements to allocate.
+ */
+static inline void CheckAllocationConstraints(size_t element_size, size_t num_elements)
+{
+	if (num_elements > SIZE_MAX / element_size) MallocError(SIZE_MAX);
+}
+
+/**
+ * Checks whether allocating memory would overflow size_t.
+ *
+ * @tparam T Structure to allocate.
+ * @param num_elements Number of elements to allocate.
+ */
+template <typename T>
+static inline void CheckAllocationConstraints(size_t num_elements)
+{
+	CheckAllocationConstraints(sizeof(T), num_elements);
+}
+
 /**
  * Simplified allocation function that allocates the specified number of
  * elements of the given type. It also explicitly casts it to the requested
@@ -42,6 +65,9 @@ static FORCEINLINE T *MallocT(size_t num_elements)
 	 */
 	if (num_elements == 0) return NULL;
 
+	/* Ensure the size does not overflow. */
+	CheckAllocationConstraints<T>(num_elements);
+
 	T *t_ptr = (T*)malloc(num_elements * sizeof(T));
 	if (t_ptr == NULL) MallocError(num_elements * sizeof(T));
 	return t_ptr;
@@ -96,12 +122,17 @@ static FORCEINLINE T *ReallocT(T *t_ptr, size_t num_elements)
 		return NULL;
 	}
 
+	/* Ensure the size does not overflow. */
+	CheckAllocationConstraints<T>(num_elements);
+
 	t_ptr = (T*)realloc(t_ptr, num_elements * sizeof(T));
 	if (t_ptr == NULL) ReallocError(num_elements * sizeof(T));
 	return t_ptr;
 }
 
 /** alloca() has to be called in the parent function, so define AllocaM() as a macro */
-#define AllocaM(T, num_elements) ((T*)alloca((num_elements) * sizeof(T)))
+#define AllocaM(T, num_elements) \
+	(CheckAllocationConstraints<T>(num_elements), \
+	(T*)alloca((num_elements) * sizeof(T)))
 
 #endif /* ALLOC_FUNC_HPP */

src/fontcache.cpp

@@ -1034,6 +1034,9 @@ const Sprite *GetGlyph(FontSize size, WChar key)
 	width  = max(1, slot->bitmap.width + (size == FS_NORMAL));
 	height = max(1, slot->bitmap.rows  + (size == FS_NORMAL));
 
+	/* Limit glyph size to prevent overflows later on. */
+	if (width > 256 || height > 256) usererror("Font glyph is too large");
+
 	/* FreeType has rendered the glyph, now we allocate a sprite and copy the image into it */
 	sprite.AllocateData(width * height);
 	sprite.width = width;

src/heightmap.cpp

@@ -142,13 +142,24 @@ static bool ReadHeightmapPNG(char *filename, uint *x, uint *y, byte **map)
 		return false;
 	}
 
+	uint width = png_get_image_width(png_ptr, info_ptr);
+	uint height = png_get_image_height(png_ptr, info_ptr);
+
+	/* Check if image dimensions don't overflow a size_t to avoid memory corruption. */
+	if ((uint64)width * height >= (size_t)-1) {
+		ShowErrorMessage(STR_ERROR_PNGMAP, STR_ERROR_HEIGHTMAP_TOO_LARGE, WL_ERROR);
+		fclose(fp);
+		png_destroy_read_struct(&png_ptr, &info_ptr, NULL);
+		return false;
+	}
+
 	if (map != NULL) {
-		*map = MallocT<byte>(png_get_image_width(png_ptr, info_ptr) * png_get_image_height(png_ptr, info_ptr));
+		*map = MallocT<byte>(width * height);
 		ReadHeightmapPNGImageData(*map, png_ptr, info_ptr);
 	}
 
-	*x = png_get_image_width(png_ptr, info_ptr);
+	*x = width;
-	*y = png_get_image_height(png_ptr, info_ptr);
+	*y = height;
 
 	fclose(fp);
 	png_destroy_read_struct(&png_ptr, &info_ptr, NULL);
@@ -243,6 +254,14 @@ static bool ReadHeightmapBMP(char *filename, uint *x, uint *y, byte **map)
 		return false;
 	}
 
+	/* Check if image dimensions don't overflow a size_t to avoid memory corruption. */
+	if ((uint64)info.width * info.height >= (size_t)-1 / (info.bpp == 24 ? 3 : 1)) {
+		ShowErrorMessage(STR_ERROR_BMPMAP, STR_ERROR_HEIGHTMAP_TOO_LARGE, WL_ERROR);
+		fclose(f);
+		BmpDestroyData(&data);
+		return false;
+	}
+
 	if (map != NULL) {
 		if (!BmpReadBitmap(&buffer, &info, &data)) {
 			ShowErrorMessage(STR_ERROR_BMPMAP, STR_ERROR_BMPMAP_IMAGE_TYPE, WL_ERROR);

src/lang/english.txt

@@ -3445,6 +3445,8 @@ STR_ERROR_PNGMAP_MISC                                           :{WHITE}... some
 STR_ERROR_BMPMAP                                                :{WHITE}Can't load landscape from BMP...
 STR_ERROR_BMPMAP_IMAGE_TYPE                                     :{WHITE}... could not convert image type
 
+STR_ERROR_HEIGHTMAP_TOO_LARGE                                   :{WHITE}... image is too large
+
 STR_WARNING_HEIGHTMAP_SCALE_CAPTION                             :{WHITE}Scale warning
 STR_WARNING_HEIGHTMAP_SCALE_MESSAGE                             :{YELLOW}Resizing source map too much is not recommended. Continue with the generation?
 

src/misc/binaryheap.hpp

@@ -198,6 +198,8 @@ public:
 	FORCEINLINE void Include(T *new_item)
 	{
 		if (this->IsFull()) {
+			assert(this->capacity < UINT_MAX / 2);
+
 			this->capacity *= 2;
 			this->data = ReallocT<T*>(this->data, this->capacity + 1);
 		}

src/misc/blob.hpp

@@ -260,6 +260,7 @@ public:
 		if (Capacity() >= new_size) return;
 		/* calculate minimum block size we need to allocate
 		 * and ask allocation policy for some reasonable block size */
+		assert(new_size < SIZE_MAX - header_size - tail_reserve);
 		new_size = AllocPolicy(header_size + new_size + tail_reserve);
 
 		/* allocate new block and setup header */

src/misc/fixedsizearray.hpp

@@ -53,6 +53,9 @@ public:
 	/** Default constructor. Preallocate space for items and header, then initialize header. */
 	FixedSizeArray()
 	{
+		/* Ensure the size won't overflow. */
+		assert_compile(C < (SIZE_MAX - HeaderSize) / Tsize);
+
 		/* allocate block for header + items (don't construct items) */
 		data = (T*)((MallocT<byte>(HeaderSize + C * Tsize)) + HeaderSize);
 		SizeRef() = 0; // initial number of items

src/openttd.cpp

@@ -577,11 +577,12 @@ int ttd_main(int argc, char *argv[])
 
 	/*
 	 * The width and height must be at least 1 pixel and width times
-	 * height must still fit within a 32 bits integer, this way all
+	 * height times bytes per pixel must still fit within a 32 bits
-	 * internal drawing routines work correctly.
+	 * integer, even for 32 bpp video modes. This way all internal
+	 * drawing routines work correctly.
 	 */
-	_cur_resolution.width  = ClampU(_cur_resolution.width,  1, UINT16_MAX);
+	_cur_resolution.width  = ClampU(_cur_resolution.width,  1, UINT16_MAX / 2);
-	_cur_resolution.height = ClampU(_cur_resolution.height, 1, UINT16_MAX);
+	_cur_resolution.height = ClampU(_cur_resolution.height, 1, UINT16_MAX / 2);
 
 #if defined(ENABLE_NETWORK)
 	if (dedicated) DEBUG(net, 0, "Starting dedicated version %s", _openttd_revision);

src/pathfinder/npf/queue.cpp

@@ -234,6 +234,9 @@ void Hash::Init(Hash_HashProc *hash, uint num_buckets)
 	/* Allocate space for the Hash, the buckets and the bucket flags */
 	uint i;
 
+	/* Ensure the size won't overflow. */
+	CheckAllocationConstraints(sizeof(*this->buckets) + sizeof(*this->buckets_in_use), num_buckets);
+
 	this->hash = hash;
 	this->size = 0;
 	this->num_buckets = num_buckets;

src/saveload/saveload.h

@@ -15,12 +15,6 @@
 #include "../fileio_type.h"
 #include "../strings_type.h"
 
-#ifdef SIZE_MAX
-#undef SIZE_MAX
-#endif
-
-#define SIZE_MAX ((size_t)-1)
-
 /** Save or load result codes. */
 enum SaveOrLoadResult {
 	SL_OK     = 0, ///< completed successfully

src/script/squirrel_helper.hpp

@@ -118,6 +118,9 @@ namespace SQConvert {
 
 	template <> inline Array      *GetParam(ForceType<Array *>,      HSQUIRRELVM vm, int index, SQAutoFreePointers *ptr)
 	{
+		/* Sanity check of the size. */
+		if (sq_getsize(vm, index) > UINT16_MAX) throw sq_throwerror(vm, _SC("an array used as parameter to a function is too large"));
+
 		SQObject obj;
 		sq_getstackobj(vm, index, &obj);
 		sq_pushobject(vm, obj);

src/sound.cpp

@@ -111,7 +111,8 @@ static bool SetBankSource(MixerChannel *mc, const SoundEntry *sound)
 {
 	assert(sound != NULL);
 
-	if (sound->file_size == 0) return false;
+	/* Check for valid sound size. */
+	if (sound->file_size == 0 || sound->file_size > ((size_t)-1) - 2) return false;
 
 	int8 *mem = MallocT<int8>(sound->file_size + 2);
 	/* Add two extra bytes so rate conversion can read these

src/sound/win32_s.cpp

@@ -15,6 +15,7 @@
 #include "../mixer.h"
 #include "../core/alloc_func.hpp"
 #include "../core/bitmath_func.hpp"
+#include "../core/math_func.hpp"
 #include "win32_s.h"
 #include <windows.h>
 #include <mmsystem.h>
@@ -63,7 +64,9 @@ const char *SoundDriver_Win32::Start(const char * const *parm)
 	wfex.nBlockAlign = (wfex.nChannels * wfex.wBitsPerSample) / 8;
 	wfex.nAvgBytesPerSec = wfex.nSamplesPerSec * wfex.nBlockAlign;
 
+	/* Limit buffer size to prevent overflows. */
 	_bufsize = GetDriverParamInt(parm, "bufsize", (GB(GetVersion(), 0, 8) > 5) ? 8192 : 4096);
+	_bufsize = min(_bufsize, UINT16_MAX);
 
 	try {
 		if (NULL == (_event = CreateEvent(NULL, FALSE, FALSE, NULL))) throw "Failed to create event";

src/spriteloader/png.cpp

@@ -106,9 +106,21 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i
 			if (strcmp("y_offs", text_ptr[i].key) == 0) sprite->y_offs = strtol(text_ptr[i].text, NULL, 0);
 		}
 
-		sprite->height = png_get_image_height(png_ptr, info_ptr);
+		uint height = png_get_image_height(png_ptr, info_ptr);
-		sprite->width  = png_get_image_width(png_ptr, info_ptr);
+		uint width  = png_get_image_width(png_ptr, info_ptr);
+		/* Check if sprite dimensions aren't larger than what is allowed in GRF-files. */
+		if (height > UINT8_MAX || width > UINT16_MAX) {
+			png_destroy_read_struct(&png_ptr, &info_ptr, &end_info);
+			return false;
+		}
+		sprite->height = height;
+		sprite->width  = width;
 		sprite->AllocateData(sprite->width * sprite->height);
+	} else if (sprite->height != png_get_image_height(png_ptr, info_ptr) || sprite->width != png_get_image_width(png_ptr, info_ptr)) {
+		/* Make sure the mask image isn't larger than the sprite image. */
+		DEBUG(misc, 0, "Ignoring mask for SpriteID %d as it isn't the same dimension as the masked sprite", id);
+		png_destroy_read_struct(&png_ptr, &info_ptr, &end_info);
+		return true;
 	}
 
 	bit_depth  = png_get_bit_depth(png_ptr, info_ptr);
@@ -116,6 +128,7 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i
 
 	if (mask && (bit_depth != 8 || colour_type != PNG_COLOR_TYPE_PALETTE)) {
 		DEBUG(misc, 0, "Ignoring mask for SpriteID %d as it isn't a 8 bit palette image", id);
+		png_destroy_read_struct(&png_ptr, &info_ptr, &end_info);
 		return true;
 	}
 

src/stdafx.h

@@ -63,6 +63,10 @@
 #include <climits>
 #include <cassert>
 
+#ifndef SIZE_MAX
+	#define SIZE_MAX ((size_t)-1)
+#endif
+
 #if defined(UNIX) || defined(__MINGW32__)
 	#include <sys/types.h>
 #endif
@@ -335,6 +339,7 @@ assert_compile(sizeof(uint64) == 8);
 assert_compile(sizeof(uint32) == 4);
 assert_compile(sizeof(uint16) == 2);
 assert_compile(sizeof(uint8)  == 1);
+assert_compile(SIZE_MAX >= UINT32_MAX);
 
 #ifndef M_PI_2
 #define M_PI_2 1.57079632679489661923