Merge branch '1392-2-2-15-attempting-to-draw-a-hud-graphic-with-the-same-lump-name-as-a-lua-script-crashes-the' into 'next'

Check if a lump is a valid patch when looking for one by name

Closes #1392

See merge request STJr/SRB2!2621

src/console.c

@@ -1737,6 +1737,8 @@ static void CON_DrawBackpic(void)
 
 	// Cache the patch.
 	con_backpic = W_CachePatchNum(piclump, PU_PATCH);
+	if (con_backpic == NULL)
+		return;
 
 	// Center the backpic, and draw a vertically cropped patch.
 	w = con_backpic->width * vid.dup;

src/hardware/hw_cache.c

@@ -483,10 +483,13 @@ static void HWR_GenerateTexture(INT32 texnum, GLMapTexture_t *grtex, GLMipmap_t
 				realpatch = W_CachePatchNumPwad(wadnum, lumpnum, PU_PATCH);
 		}
 
-		HWR_DrawTexturePatchInCache(mipmap, blockwidth, blockheight, texture, patch, realpatch);
+		if (realpatch != NULL)
+		{
+			HWR_DrawTexturePatchInCache(mipmap, blockwidth, blockheight, texture, patch, realpatch);
 
-		if (free_patch)
-			Patch_Free(realpatch);
+			if (free_patch)
+				Patch_Free(realpatch);
+		}
 	}
 	//Hurdler: not efficient at all but I don't remember exactly how HWR_DrawPatchInCache works :(
 	if (format2bpp(mipmap->format)==4)

src/r_patch.c

@@ -31,14 +31,12 @@ patch_t *Patch_Create(INT16 width, INT16 height)
 	return patch;
 }
 
-patch_t *Patch_CreateFromDoomPatch(softwarepatch_t *source)
+patch_t *Patch_CreateFromDoomPatch(softwarepatch_t *source, size_t insize)
 {
-	patch_t *patch = Patch_Create(0, 0);
-	if (!source)
-		return patch;
+	if (!Picture_CheckIfDoomPatch(source, insize))
+		return NULL;
 
-	patch->width      = SHORT(source->width);
-	patch->height     = SHORT(source->height);
+	patch_t *patch = Patch_Create(SHORT(source->width), SHORT(source->height));
 	patch->leftoffset = SHORT(source->leftoffset);
 	patch->topoffset  = SHORT(source->topoffset);
 

src/r_patch.h

@@ -19,7 +19,7 @@
 
 // Patch functions
 patch_t *Patch_Create(INT16 width, INT16 height);
-patch_t *Patch_CreateFromDoomPatch(softwarepatch_t *source);
+patch_t *Patch_CreateFromDoomPatch(softwarepatch_t *source, size_t insize);
 void Patch_CalcDataSizes(softwarepatch_t *source, size_t *total_pixels, size_t *total_posts);
 void Patch_MakeColumns(softwarepatch_t *source, size_t num_columns, INT16 width, UINT8 *pixels, column_t *columns, post_t *posts, boolean flip);
 void Patch_Free(patch_t *patch);

src/r_picformats.c

@@ -87,7 +87,12 @@ void *Picture_Convert(
 	(void)insize;
 
 	if (Picture_IsPatchFormat(outformat))
-		return Picture_PatchConvert(informat, picture, outformat, outsize, inwidth, inheight, inleftoffset, intopoffset, flags);
+	{
+		void *converted = Picture_PatchConvert(informat, insize, picture, outformat, outsize, inwidth, inheight, inleftoffset, intopoffset, flags);
+		if (converted == NULL)
+			I_Error("Picture_Convert: conversion to patch did not result in a valid graphic");
+		return converted;
+	}
 	else if (Picture_IsFlatFormat(outformat))
 		return Picture_FlatConvert(informat, picture, outformat, outsize, inwidth, intopoffset, flags);
 	else
@@ -235,8 +240,8 @@ static void *WritePatchPixel_i8o8(void *ptr, void *input)
   * \return A pointer to the converted picture.
   */
 void *Picture_PatchConvert(
-	pictureformat_t informat, void *picture, pictureformat_t outformat,
-	size_t *outsize,
+	pictureformat_t informat, size_t insize, void *picture,
+	pictureformat_t outformat, size_t *outsize,
 	INT32 inwidth, INT32 inheight, INT32 inleftoffset, INT32 intopoffset,
 	pictureflags_t flags)
 {
@@ -245,7 +250,7 @@ void *Picture_PatchConvert(
 	{
 		if (outsize != NULL)
 			*outsize = sizeof(patch_t);
-		return Patch_CreateFromDoomPatch(picture);
+		return Patch_CreateFromDoomPatch(picture, insize);
 	}
 
 	INT32 outbpp = Picture_FormatBPP(outformat);
@@ -799,47 +804,44 @@ boolean Picture_IsFlatFormat(pictureformat_t format)
 }
 
 /** Returns true if the lump is a valid Doom patch.
-  * PICFMT_DOOMPATCH only.
   *
   * \param patch Input patch.
   * \param picture Input patch size.
-  * \return True if the input patch is valid.
+  * \return True if the input patch is valid, false if not.
   */
 boolean Picture_CheckIfDoomPatch(softwarepatch_t *patch, size_t size)
 {
-	INT16 width, height;
-	boolean result;
-
-	// minimum length of a valid Doom patch
-	if (size < 13)
+	// Does not meet minimum size requirements
+	if (size < MIN_PATCH_LUMP_SIZE)
 		return false;
 
-	width = SHORT(patch->width);
-	height = SHORT(patch->height);
-	result = (height > 0 && height <= 16384 && width > 0 && width <= 16384);
+	INT16 width = SHORT(patch->width);
+	INT16 height = SHORT(patch->height);
 
-	if (result)
+	// Quickly check for the dimensions first.
+	if (width <= 0 || height <= 0 || width > MAX_PATCH_DIMENSIONS || height > MAX_PATCH_DIMENSIONS)
+		return false;
+
+	// Lump size makes no sense given the width
+	if (!VALID_PATCH_LUMP_SIZE(size, width))
+		return false;
+
+	// The dimensions seem like they might be valid for a patch, so
+	// check the column directory for extra security. All columns
+	// must begin after the column directory, and none of them must
+	// point past the end of the patch.
+	for (INT16 x = 0; x < width; x++)
 	{
-		// The dimensions seem like they might be valid for a patch, so
-		// check the column directory for extra security. All columns
-		// must begin after the column directory, and none of them must
-		// point past the end of the patch.
-		INT16 x;
+		UINT32 ofs = LONG(patch->columnofs[x]);
 
-		for (x = 0; x < width; x++)
+		// Need one byte for an empty column (but there's patches that don't know that!)
+		if (ofs < FIRST_PATCH_LUMP_COLUMN(width) || (size_t)ofs >= size)
 		{
-			UINT32 ofs = LONG(patch->columnofs[x]);
-
-			// Need one byte for an empty column (but there's patches that don't know that!)
-			if (ofs < (UINT32)width * 4 + 8 || ofs >= (UINT32)size)
-			{
-				result = false;
-				break;
-			}
+			return false;
 		}
 	}
 
-	return result;
+	return true;
 }
 
 /** Converts a texture to a flat.
@@ -901,12 +903,13 @@ void *Picture_TextureToFlat(size_t texnum)
   */
 boolean Picture_IsLumpPNG(const UINT8 *d, size_t s)
 {
-	if (s < 67) // https://web.archive.org/web/20230524232139/http://garethrees.org/2007/11/14/pngcrush/
+	if (s < PNG_MIN_SIZE)
 		return false;
+
 	// Check for PNG file signature using memcmp
 	// As it may be faster on CPUs with slow unaligned memory access
 	// Ref: http://www.libpng.org/pub/png/spec/1.2/PNG-Rationale.html#R.PNG-file-signature
-	return (memcmp(&d[0], "\x89\x50\x4e\x47\x0d\x0a\x1a\x0a", 8) == 0);
+	return (memcmp(&d[0], "\x89\x50\x4e\x47\x0d\x0a\x1a\x0a", PNG_HEADER_SIZE) == 0);
 }
 
 #ifndef NO_PNG_LUMPS
@@ -1373,7 +1376,9 @@ void *Picture_PNGConvert(
 		}
 
 		// Now, convert it!
-		converted = Picture_PatchConvert(informat, flat, outformat, outsize, (INT32)width, (INT32)height, *leftoffset, *topoffset, flags);
+		converted = Picture_PatchConvert(informat, flatsize, flat, outformat, outsize, (INT32)width, (INT32)height, *leftoffset, *topoffset, flags);
+		if (converted == NULL)
+			I_Error("Picture_PNGConvert: conversion to patch did not result in a valid graphic");
 		Z_Free(flat);
 		return converted;
 	}

src/r_picformats.h

@@ -55,6 +55,29 @@ enum
 	PICDEPTH_32BPP = 32
 };
 
+// Maximum allowed dimensions for a patch
+#define MAX_PATCH_DIMENSIONS 8192
+
+// Minimum amount of bytes required for a valid patch lump header
+#define MIN_PATCH_LUMP_HEADER_SIZE ((sizeof(INT16) * 4) + sizeof(INT32))
+
+// Minimum length of a valid Doom patch lump
+// This is the size of a 1x1 patch.
+#define MIN_PATCH_LUMP_SIZE (MIN_PATCH_LUMP_HEADER_SIZE + 1)
+
+// Gets the offset to the very first column in a patch lump
+#define FIRST_PATCH_LUMP_COLUMN(width) ((sizeof(INT16) * 4) + ((width) * sizeof(INT32)))
+
+// Checks if the size of a lump is valid for a patch, given a certain width
+#define VALID_PATCH_LUMP_SIZE(lumplen, width) ((lumplen) >= FIRST_PATCH_LUMP_COLUMN(width))
+
+// Minimum size of a PNG file.
+// See: https://web.archive.org/web/20230524232139/http://garethrees.org/2007/11/14/pngcrush/
+#define PNG_MIN_SIZE 67
+
+// Size of a PNG header
+#define PNG_HEADER_SIZE 8
+
 void *Picture_Convert(
 	pictureformat_t informat, void *picture, pictureformat_t outformat,
 	size_t insize, size_t *outsize,
@@ -62,8 +85,8 @@ void *Picture_Convert(
 	pictureflags_t flags);
 
 void *Picture_PatchConvert(
-	pictureformat_t informat, void *picture, pictureformat_t outformat,
-	size_t *outsize,
+	pictureformat_t informat, size_t insize, void *picture,
+	pictureformat_t outformat, size_t *outsize,
 	INT32 inwidth, INT32 inheight, INT32 inleftoffset, INT32 intopoffset,
 	pictureflags_t flags);
 void *Picture_FlatConvert(
@@ -104,9 +127,6 @@ typedef struct
 	boolean available;
 } spriteinfo_t;
 
-// PNG support
-#define PNG_HEADER_SIZE 8
-
 boolean Picture_IsLumpPNG(const UINT8 *d, size_t s);
 
 #ifndef NO_PNG_LUMPS

src/r_textures.c

@@ -430,6 +430,10 @@ UINT8 *R_GenerateTexture(size_t texnum)
 				realpatch = W_CachePatchNumPwad(wadnum, lumpnum, PU_PATCH);
 		}
 
+		// Well, it's not valid...
+		if (realpatch == NULL)
+			continue;
+
 		x1 = patch->originx;
 		width = realpatch->width;
 		height = realpatch->height;

src/w_wad.c

@@ -1695,12 +1695,75 @@ lumpnum_t W_GetNumForLongName(const char *name)
 	return i;
 }
 
+// Checks if a given lump might be a valid patch.
+// This will need to read a bit of the file, but it attempts to not load it
+// in its entirety.
+static boolean W_IsProbablyValidPatch(UINT16 wadnum, UINT16 lumpnum)
+{
+	UINT8 header[MIN_PATCH_LUMP_HEADER_SIZE];
+
+	I_StaticAssert(sizeof(header) >= PNG_HEADER_SIZE);
+
+	// Check the file's size first
+	size_t lumplen = W_LumpLengthPwad(wadnum, lumpnum);
+
+	// Cannot be a valid Doom patch
+	if (lumplen < MIN_PATCH_LUMP_SIZE)
+		return false;
+
+	// Check if it's probably a valid PNG
+	if (lumplen >= PNG_MIN_SIZE)
+	{
+		// Read the PNG's header
+		W_ReadLumpHeaderPwad(wadnum, lumpnum, header, PNG_HEADER_SIZE, 0);
+
+		if (Picture_IsLumpPNG(header, lumplen))
+		{
+			// Assume it is if the signature matches.
+			return true;
+		}
+
+		// Otherwise, we read it as a patch
+	}
+
+	// Read the first 12 bytes
+	W_ReadLumpHeaderPwad(wadnum, lumpnum, header, sizeof(header), 0);
+
+	softwarepatch_t patch;
+	memcpy(&patch, header, sizeof(header));
+
+	INT16 width = SHORT(patch.width);
+	INT16 height = SHORT(patch.height);
+
+	// Lump size makes no sense given the width
+	if (!VALID_PATCH_LUMP_SIZE(lumplen, width))
+		return false;
+
+	// Check the dimensions.
+	if (width > 0 && height > 0 && width <= MAX_PATCH_DIMENSIONS && height <= MAX_PATCH_DIMENSIONS)
+	{
+		// Dimensions seem to make sense... But check at least the first column.
+		UINT32 ofs = LONG(patch.columnofs[0]);
+
+		// Need one byte for an empty column (but there's patches that don't know that!)
+		if (ofs < FIRST_PATCH_LUMP_COLUMN(width) || (size_t)ofs >= lumplen)
+		{
+			return false;
+		}
+
+		return true;
+	}
+
+	// Not valid if this point was reached
+	return false;
+}
+
 //
 // Same as W_CheckNumForNamePwad, but handles namespaces.
 //
 static UINT16 W_CheckNumForPatchNamePwad(const char *name, UINT16 wad, boolean longname)
 {
-	UINT16 i, start = INT16_MAX, end = INT16_MAX;
+	UINT16 i;
 	static char uname[8 + 1] = { 0 };
 	UINT32 hash = 0;
 	lumpinfo_t *lump_p;
@@ -1715,51 +1778,15 @@ static UINT16 W_CheckNumForPatchNamePwad(const char *name, UINT16 wad, boolean l
 		hash = quickncasehash(uname, 8);
 	}
 
-	// SRB2 doesn't have a specific namespace for graphics, which means someone can do weird things
-	// like placing graphics inside a namespace it doesn't make sense for them to be in, like Sounds/ or SOC/
-	// So for now, this checks for lumps OUTSIDE of the flats namespace.
-	// When this situation changes, change the loops below to check for lumps INSIDE the namespaces to look in.
-	// TODO: cache namespace lump IDs
-	if (W_FileHasFolders(wadfiles[wad]))
-	{
-		start = W_CheckNumForFolderStartPK3("Flats/", wad, 0);
-		end = W_CheckNumForFolderEndPK3("Flats/", wad, start);
-
-		// if the start and end is the same, the folder is empty
-		if (end <= start)
-		{
-			start = INT16_MAX;
-			end = INT16_MAX;
-		}
-	}
-	else
-	{
-		start = W_CheckNumForMarkerStartPwad("F_START", wad, 0);
-		end = W_CheckNumForNamePwad("F_END", wad, start);
-		if (end != INT16_MAX)
-			end++;
-	}
-
 	lump_p = wadfiles[wad]->lumpinfo;
 
-	if (start == INT16_MAX)
-		start = wadfiles[wad]->numlumps;
-
-	for (i = 0; i < start; i++, lump_p++)
+	for (i = 0; i < wadfiles[wad]->numlumps; i++, lump_p++)
 	{
 		if ((!longname && lump_p->hash == hash && !strncmp(lump_p->name, uname, sizeof(uname) - 1))
 		|| (longname && stricmp(lump_p->longname, name) == 0))
-			return i;
-	}
-
-	if (end != INT16_MAX && start < end)
-	{
-		lump_p = wadfiles[wad]->lumpinfo + end;
-
-		for (i = end; i < wadfiles[wad]->numlumps; i++, lump_p++)
 		{
-			if ((!longname && lump_p->hash == hash && !strncmp(lump_p->name, uname, sizeof(uname) - 1))
-			|| (longname && stricmp(lump_p->longname, name) == 0))
+			// Found the patch by name, but needs to check if it is valid.
+			if (W_IsProbablyValidPatch(wad, i))
 				return i;
 		}
 	}
@@ -2383,9 +2410,12 @@ static void *W_GetPatchPwad(UINT16 wad, UINT16 lump, INT32 tag)
 #endif
 		}
 
-		dest = Patch_CreateFromDoomPatch(ptr);
+		dest = Patch_CreateFromDoomPatch(ptr, len);
 		Z_Free(ptr);
 
+		if (dest == NULL)
+			return NULL;
+
 		Z_ChangeTag(dest, tag);
 		Z_SetUser(dest, &lumpcache[lump]);
 	}
@@ -2403,7 +2433,7 @@ void *W_CachePatchNumPwad(UINT16 wad, UINT16 lump, INT32 tag)
 	patch_t *patch = W_GetPatchPwad(wad, lump, tag);
 
 #ifdef HWRENDER
-	if (rendermode == render_opengl)
+	if (patch != NULL && rendermode == render_opengl)
 		Patch_CreateGL(patch);
 #endif