Address initial review comments + associated cleanup

- Grab thumbnail and minimap pictures even for base game maps
- Repair modified game event for replacement map load
- PU_STATIC thumbnail and minimap, freed on new file load lump discovery
- Fix W_CheckNumForMap to not SIGSEGV if no match
- Reduce the number of pointless map lump hunts, we've already got it stored
- Prevent use-after-free for map pictures on voting screen
- Cache "BLANKLVL" patch once
- Draw minimap on the time attack menu

Unfortunately cups are still broken and I'm not sure where to start on that, or if it's just not having a sample wad to test with that includes cups

src/d_main.c

@@ -1190,15 +1190,10 @@ D_ConvertVersionNumbers (void)
 //
 void D_SRB2Main(void)
 {
-	INT32 numbasemapheaders;
-	INT32 i;
-	UINT16 wadnum;
-	char *name;
-	virtres_t *virtmap;
-	virtlump_t *minimap, *thumbnailPic;
-
 	INT32 p;
 
+	INT32 numbasemapheaders;
+
 	INT32 pstartmap = 1;
 	boolean autostart = false;
 
@@ -1441,6 +1436,11 @@ void D_SRB2Main(void)
 
 #endif //ifndef DEVELOP
 
+	//
+	// search for mainwad maps
+	//
+	P_InitMapData(0);
+
 	numbasemapheaders = nummapheaders;
 
 	CON_SetLoadingProgress(LOADED_IWAD);
@@ -1450,40 +1450,9 @@ void D_SRB2Main(void)
 	D_CleanFile(startuppwads);
 
 	//
-	// search for maps
+	// search for pwad maps
 	//
-	for (wadnum = mainwads+1; wadnum < numwadfiles; wadnum++)
+	P_InitMapData(numbasemapheaders);
-	{
-		for (i = 0; i < numbasemapheaders; ++i)
-		{
-			name = mapheaderinfo[i]->lumpname;
-			mapheaderinfo[i]->lumpnum = W_CheckNumForMap(name);
-
-			// Get map thumbnail and minimap
-			virtmap = vres_GetMap(mapheaderinfo[i]->lumpnum);
-			thumbnailPic = vres_Find(virtmap, "PICTURE");
-			minimap = vres_Find(virtmap, "MINIMAP");
-
-			if (thumbnailPic)
-			{
-				mapheaderinfo[i]->thumbnailPic = vres_GetPatch(thumbnailPic, PU_CACHE);
-			}
-
-			if (minimap)
-			{
-				mapheaderinfo[i]->minimapPic = vres_GetPatch(minimap, PU_HUDGFX);
-			}
-
-			vres_Free(virtmap);
-
-			if (W_CheckNumForMapPwad(name, wadnum, 0) != INT16_MAX)
-			{
-				G_SetGameModified(multiplayer, true); // oops, double-defined - no record attack privileges for you
-
-				CONS_Printf("%s\n", name);
-			}
-		}
-	}
 
 	CON_SetLoadingProgress(LOADED_PWAD);
 

src/g_game.c

@@ -597,20 +597,7 @@ void G_SetGameModified(boolean silent, boolean major)
   */
 const char *G_BuildMapName(INT32 map)
 {
-#if 0
+	if (map > 0 && map <= nummapheaders && mapheaderinfo[map - 1] != NULL)
-	if (map == 0) // hack???
-	{
-		if (gamestate == GS_TITLESCREEN)
-			map = -1;
-		else if (gamestate == GS_LEVEL)
-			map = gamemap-1;
-		else
-			map = prevmap;
-		map = G_RandMap(G_TOLFlag(cv_newgametype.value), map, 0, 0, false, NULL)+1;
-	}
-#endif
-
-	if (map > 0 && map <= NUMMAPS && mapheaderinfo[map - 1] != NULL)
 	{
 		return mapheaderinfo[map - 1]->lumpname;
 	}
@@ -4612,7 +4599,7 @@ void G_InitNew(UINT8 pencoremode, INT32 map, boolean resetplayer, boolean skippr
 	// internal game map
 	// well this check is useless because it is done before (d_netcmd.c::command_map_f)
 	// but in case of for demos....
-	if (W_CheckNumForName(mapname) == LUMPERROR)
+	if (mapheaderinfo[map-1]->lumpnum == LUMPERROR)
 	{
 		I_Error("Internal game map '%s' not found\n", mapname);
 		Command_ExitGame_f();

src/hu_stuff.c

@@ -93,6 +93,7 @@ static char hu_tick;
 //-------------------------------------------
 
 patch_t *missingpat;
+patch_t *blanklvl;
 
 // song credits
 static patch_t *songcreditbg;
@@ -180,6 +181,8 @@ void HU_LoadGraphics(void)
 
 	Font_Load();
 
+	HU_UpdatePatch(&blanklvl, "BLANKLVL");
+
 	HU_UpdatePatch(&songcreditbg, "K_SONGCR");
 
 	// cache ping gfx:

src/k_menudraw.c

@@ -1855,7 +1855,7 @@ static void M_DrawCupPreview(INT16 y, cupheader_t *cup)
 			}
 
 			if (!PictureOfLevel)
-				PictureOfLevel = W_CachePatchName("BLANKLVL", PU_CACHE);
+				PictureOfLevel = blanklvl;
 
 			V_DrawSmallScaledPatch(x + 1, y+2, 0, PictureOfLevel);
 			i = (i+1) % cup->numlevels;
@@ -2087,7 +2087,7 @@ static void M_DrawLevelSelectBlock(INT16 x, INT16 y, INT16 map, boolean redblink
 	}
 
 	if (!PictureOfLevel)
-		PictureOfLevel = W_CachePatchName("BLANKLVL", PU_CACHE);
+		PictureOfLevel = blanklvl;
 
 	if (redblink)
 		V_DrawScaledPatch(3+x, y, 0, W_CachePatchName("LVLSEL2", PU_CACHE));
@@ -2163,12 +2163,10 @@ void M_DrawTimeAttack(void)
 	{
 		if (mapheaderinfo[map])
 		{
-			minimap = mapheaderinfo[map]->minimapPic;
+			if ((minimap = mapheaderinfo[map]->minimapPic))
+				V_DrawScaledPatch(24-t, 82, 0, minimap);
 		}
 
-		if (!minimap)
-			V_DrawScaledPatch(24-t, 82, 0, minimap);
-
 		V_DrawRightAlignedString(rightedge-12, 82, highlightflags, "BEST LAP:");
 		K_drawKartTimestamp(0, 162+t, 88, 0, 2);
 

src/p_setup.c

@@ -365,13 +365,13 @@ static void P_ClearSingleMapHeaderInfo(INT16 i)
 
 	if (mapheaderinfo[num]->thumbnailPic)
 	{
-		Z_Free(mapheaderinfo[num]->thumbnailPic);
+		Patch_Free(mapheaderinfo[num]->thumbnailPic);
 		mapheaderinfo[num]->thumbnailPic = NULL;
 	}
 
 	if (mapheaderinfo[num]->minimapPic)
 	{
-		Z_Free(mapheaderinfo[num]->minimapPic);
+		Patch_Free(mapheaderinfo[num]->minimapPic);
 		mapheaderinfo[num]->minimapPic = NULL;
 	}
 
@@ -4339,8 +4339,8 @@ boolean P_LoadLevel(boolean fromnetsave, boolean reloadinggamestate)
 	}
 
 	// internal game map
-	maplumpname = G_BuildMapName(gamemap);
+	maplumpname = mapheaderinfo[gamemap-1]->lumpname;
-	lastloadedmaplumpnum = W_CheckNumForMap(maplumpname);
+	lastloadedmaplumpnum = mapheaderinfo[gamemap-1]->lumpnum;
 	if (lastloadedmaplumpnum == LUMPERROR)
 		I_Error("Map %s not found.\n", maplumpname);
 
@@ -4655,6 +4655,87 @@ static lumpinfo_t* FindFolder(const char *folName, UINT16 *start, UINT16 *end, l
 	return lumpinfo;
 }
 
+// Initialising map data (and catching replacements)...
+UINT8 P_InitMapData(INT32 numexistingmapheaders)
+{
+	UINT8 ret = 0;
+	INT32 i;
+	lumpnum_t maplump;
+	virtres_t *virtmap;
+	virtlump_t *minimap, *thumbnailPic;
+	char *name;
+
+	for (i = 0; i < nummapheaders; ++i)
+	{
+		name = mapheaderinfo[i]->lumpname;
+		maplump = W_CheckNumForMap(name);
+
+		// Doesn't exist?
+		if (maplump == INT16_MAX)
+		{
+#ifndef DEVELOP
+			if (!numexistingmapheaders)
+			{
+				I_Error("P_InitMapData: Base map %s has a header but no level\n", name);
+			}
+#endif
+			continue;
+		}
+
+		// No change?
+		if (mapheaderinfo[i]->lumpnum == maplump)
+			continue;
+
+		// Okay, it does...
+		{
+			ret |= MAPRET_ADDED;
+			CONS_Printf("%s\n", name);
+
+			if (numexistingmapheaders && mapheaderinfo[i]->lumpnum != LUMPERROR)
+			{
+				G_SetGameModified(multiplayer, true); // oops, double-defined - no record attack privileges for you
+
+				//If you replaced the map you're on, end the level when done.
+				if (i == gamemap - 1)
+					ret |= MAPRET_CURRENTREPLACED;
+			}
+
+			mapheaderinfo[i]->lumpnum = maplump;
+
+			// Get map thumbnail and minimap
+			virtmap = vres_GetMap(mapheaderinfo[i]->lumpnum);
+			thumbnailPic = vres_Find(virtmap, "PICTURE");
+			minimap = vres_Find(virtmap, "MINIMAP");
+
+			// Clear out existing graphics...
+			if (mapheaderinfo[i]->thumbnailPic)
+			{
+				Patch_Free(mapheaderinfo[i]->thumbnailPic);
+			}
+
+			if (mapheaderinfo[i]->minimapPic)
+			{
+				Patch_Free(mapheaderinfo[i]->minimapPic);
+			}
+
+			// Now apply the new ones!
+			if (thumbnailPic)
+			{
+				mapheaderinfo[i]->thumbnailPic = vres_GetPatch(thumbnailPic, PU_STATIC);
+			}
+
+			if (minimap)
+			{
+				mapheaderinfo[i]->minimapPic = vres_GetPatch(minimap, PU_STATIC);
+			}
+
+			vres_Free(virtmap);
+		}
+	}
+
+	return ret;
+}
+
 UINT16 p_adding_file = INT16_MAX;
 
 //
@@ -4665,16 +4746,12 @@ boolean P_AddWadFile(const char *wadfilename)
 {
 	size_t i, j, sreplaces = 0, mreplaces = 0, digmreplaces = 0;
 	INT32 numexistingmapheaders = nummapheaders;
-	INT32 map;
 	UINT16 numlumps, wadnum;
 	char *name;
 	lumpinfo_t *lumpinfo;
-	virtres_t *virtmap;
-	virtlump_t *minimap, *thumbnailPic;
 
 	//boolean texturechange = false; ///\todo Useless; broken when back-frontporting PK3 changes?
-	boolean mapsadded = false;
+	UINT8 mapsadded = 0;
-	boolean replacedcurrentmap = false;
 
 	// Vars to help us with the position start and amount of each resource type.
 	// Useful for PK3s since they use folders.
@@ -4824,44 +4901,7 @@ boolean P_AddWadFile(const char *wadfilename)
 	//
 	// search for maps
 	//
-	for (map = 0; map < numexistingmapheaders; ++map)
+	mapsadded = P_InitMapData(numexistingmapheaders);
-	{
-		name = mapheaderinfo[map]->lumpname;
-
-		if (mapheaderinfo[i]->lumpnum != W_CheckNumForMap(name))
-		{
-			mapheaderinfo[i]->lumpnum = W_CheckNumForMap(name);
-
-			// Get map thumbnail and minimap
-			virtmap = vres_GetMap(mapheaderinfo[i]->lumpnum);
-			thumbnailPic = vres_Find(virtmap, "PICTURE");
-			minimap = vres_Find(virtmap, "MINIMAP");
-
-			if (thumbnailPic)
-			{
-				mapheaderinfo[i]->thumbnailPic = vres_GetPatch(thumbnailPic, PU_CACHE);
-			}
-
-			if (minimap)
-			{
-				mapheaderinfo[i]->minimapPic = vres_GetPatch(minimap, PU_HUDGFX);
-			}
-
-			vres_Free(virtmap);
-		}
-
-		if (W_CheckNumForMapPwad(name, wadnum, 0) != INT16_MAX)
-		{
-			G_SetGameModified(multiplayer, true); // oops, double-defined - no record attack privileges for you
-
-			//If you replaced the map you're on, end the level when done.
-			if (map == gamemap - 1)
-				replacedcurrentmap = true;
-
-			CONS_Printf("%s\n", name);
-			mapsadded = true;
-		}
-	}
 
 	if (!mapsadded)
 		CONS_Printf(M_GetText("No maps added\n"));
@@ -4880,7 +4920,7 @@ boolean P_AddWadFile(const char *wadfilename)
 	if (cursaveslot > 0)
 		cursaveslot = 0;
 
-	if (replacedcurrentmap && gamestate == GS_LEVEL && (netgame || multiplayer))
+	if ((mapsadded & MAPRET_CURRENTREPLACED) && gamestate == GS_LEVEL && (netgame || multiplayer))
 	{
 		CONS_Printf(M_GetText("Current map %d replaced by added file, ending the level to ensure consistency.\n"), gamemap);
 		if (server)

src/p_setup.h

@@ -108,6 +108,11 @@ boolean P_LoadLevel(boolean fromnetsave, boolean reloadinggamestate);
 void HWR_LoadLevel(void);
 #endif
 boolean P_AddWadFile(const char *wadfilename);
+
+#define MAPRET_ADDED (1)
+#define MAPRET_CURRENTREPLACED (1<<1)
+UINT8 P_InitMapData(INT32 numexistingmapheaders);
+
 boolean P_RunSOC(const char *socfilename);
 void P_LoadSoundsRange(UINT16 wadnum, UINT16 first, UINT16 num);
 void P_LoadMusicsRange(UINT16 wadnum, UINT16 first, UINT16 num);

src/r_defs.h

@@ -698,6 +698,7 @@ typedef struct
 } patch_t;
 
 extern patch_t *missingpat;
+extern patch_t *blanklvl;
 
 #if defined(_MSC_VER)
 #pragma pack(1)

src/w_wad.c

@@ -1274,7 +1274,7 @@ lumpnum_t W_CheckNumForMap(const char *name)
 		}
 	}
 
-	for (i = numwadfiles - 1; i < numwadfiles; i--)
+	for (i = numwadfiles - 1; i >= 0; i--)
 	{
 		check = W_CheckNumForMapPwad(name, (UINT16)i, 0);
 

src/y_inter.c

@@ -111,7 +111,6 @@ typedef struct
 	char str[62];
 	UINT8 gtc;
 	const char *gts;
-	patch_t *pic;
 	boolean encore;
 } y_votelvlinfo;
 
@@ -1010,7 +1009,18 @@ void Y_VoteDrawer(void)
 		else
 		{
 			str = levelinfo[i].str;
-			pic = levelinfo[i].pic;
+
+			pic = NULL;
+
+			if (mapheaderinfo[votelevels[i][0]])
+			{
+				pic = mapheaderinfo[votelevels[i][0]]->thumbnailPic;
+			}
+
+			if (!pic)
+			{
+				pic = blanklvl;
+			}
 		}
 
 		if (selected[i])
@@ -1130,9 +1140,23 @@ void Y_VoteDrawer(void)
 			patch_t *pic;
 
 			if (votes[i] >= 3 && (i != pickedvote || voteendtic == -1))
+			{
 				pic = randomlvl;
+			}
 			else
-				pic = levelinfo[votes[i]].pic;
+			{
+				pic = NULL;
+
+				if (mapheaderinfo[votelevels[votes[i]][0]])
+				{
+					pic = mapheaderinfo[votelevels[votes[i]][0]]->thumbnailPic;
+				}
+
+				if (!pic)
+				{
+					pic = blanklvl;
+				}
+			}
 
 			if (!timer && i == voteclient.ranim)
 			{
@@ -1530,18 +1554,6 @@ void Y_StartVote(void)
 			levelinfo[i].gts = Gametype_Names[votelevels[i][1]];
 		else
 			levelinfo[i].gts = NULL;
-
-		// set up the pic
-		patch_t *thumbnailPic = NULL;
-		if (mapheaderinfo[votelevels[i][0]+1])
-		{
-			thumbnailPic = mapheaderinfo[votelevels[i][0]]->thumbnailPic;
-		}
-
-		if (thumbnailPic)
-			levelinfo[i].pic = thumbnailPic;
-		else
-			levelinfo[i].pic = W_CachePatchName("BLANKLVL", PU_STATIC);
 	}
 
 	voteclient.loaded = true;
@@ -1561,8 +1573,6 @@ void Y_EndVote(void)
 //
 static void Y_UnloadVoteData(void)
 {
-	UINT8 i;
-
 	voteclient.loaded = false;
 
 	if (rendermode != render_soft)
@@ -1577,28 +1587,6 @@ static void Y_UnloadVoteData(void)
 	UNLOAD(cursor4);
 	UNLOAD(randomlvl);
 	UNLOAD(rubyicon);
-
-	// to prevent double frees...
-	for (i = 0; i < 4; i++)
-	{
-	// I went to all the trouble of doing this,
-	// but literally nowhere else frees level pics.
-#if 0
-		UINT8 j;
-
-		if (!levelinfo[i].pic)
-			continue;
-
-		for (j = i+1; j < 4; j++)
-		{
-			if (levelinfo[j].pic == levelinfo[i].pic)
-				levelinfo[j].pic = NULL;
-		}
-		UNLOAD(levelinfo[i].pic);
-#else
-		CLEANUP(levelinfo[i].pic);
-#endif
-	}
 }
 
 //