Game crashes when engine reuses brush model slot for studio model

[SamVanheer]

When the engine loads models it tries to find a slot in the mod_known list (code reconstructed from the Linux version of the engine):

model_t* Mod_FindName(qboolean trackCRC, const char *name)
{
	if ( !*name )
		Sys_Error("Mod_FindName: NULL name");

	model_t* fallback = nullptr;
	int i = 0;
	model_t* mod;
  
	for ( i = 0, mod = mod_known; i < mod_numknown; ++i, ++mod )
	{
		// Exact match
		if ( !Q_strcmp(mod->name, name) )
			break;
	  
		// Slot has not been loaded into memory yet
		if ( mod->needload == 2 )
		{
			// If all slots are assigned, reuse the first slot that hasn't loaded yet
			if ( !fallback )
			{
				fallback = mod;
			}
			// Only allow these to be used as a fallback if they aren't sprites or brush models
			else if ( mod->type != mod_sprite && mod->type != mod_brush )
			{
				fallback = mod;
			}
		}
	}
  
	if ( i == mod_numknown )
	{
		if ( i > (MAX_KNOWN_MODELS - 1) )
		{
			// All slots in active use
			if ( !fallback )
				Sys_Error("mod_numknown >= MAX_KNOWN_MODELS");
			
			// Reuse a slot previously used by another model
			mod = fallback;			
			i = fallback - mod_known;
		}
		else
		{
			mod_numknown = i + 1;
		}
		
		mod_known_info[i].firstCRCDone = 0;
		mod_known_info[i].initialCRC = 0;
		mod_known_info[i].shouldCRC = trackCRC;
		
		Q_strncpy(mod->name, name, sizeof(mod->name) - 1);
		mod->name[sizeof(mod->name) - 1] = 0;
		
		if ( mod->needload != 3 )
			mod->needload = 1;
	}
	
	return mod;
}

The problem is that when this function reuses a slot that was previously a brush model for a studio model then the model's variables are not cleared out and still refer to now-invalid memory addresses.

The engine reuses slots if all slots are in use so this problem only becomes apparent when 1024 unique models have been loaded before. Most games use the same set of models in all maps, so this is more likely to affect mods that have a lot of custom models used in individual maps. This also includes brush models which always have the same name, meaning the first brush model in a map is always named *1 so it reuses that slot every time. If one map has a lot of brush models then subsequent maps precaching models can reuse those slots.

On both Windows and Linux this causes problems that lead to the game shutting down, either due to attempting to allocate a negative amount of memory from the engine's own memory pool (Windows) or when attempting to generate lightmaps (Linux):

In GL_BuildLightmaps the engine generates lightmaps for all models that have surfaces. Since the studio model's surfaces variable has not been cleared the engine will happily access the effectively random memory address and treats it as a list of surfaces. Since the list of surfaces is allocated from the engine's hunk memory pool this address is still valid so this won't cause a segfault.

When GL_CreateSurfaceLightmap tries to access the surface's texinfo member it is reinterpreting the data now stored in that region of hunk memory as a memory address, and this address is likely to point to invalid memory which does cause a segfault.

The code above shows the edge case that allows this to happen:

// If all slots are assigned, reuse the first slot that hasn't loaded yet
if ( !fallback )
{
	fallback = mod;
}
// Only allow these to be used as a fallback if they aren't sprites or brush models
else if ( mod->type != mod_sprite && mod->type != mod_brush )
{
	fallback = mod;
}

If the first free slot is a brush model and the model it is assigned to is a studio model this will cause the problem.

To fix this Mod_ClearAll should zero out the entire model object and restore only these three member variables:

• name
• needload
• type

The cache member may also need restoring, that depends on whether the engine resets the cache member when reloading the model.

By zeroing out that data there can't be any dangling pointers to cause problems.

It may be easier to simply zero out the entire mod_known list since there is very little advantage in keeping that data around. Anything capable of running Steam Half-Life (Windows 7 or newer, Windows 10 or newer in a few months) can easily handle the insignificant amount of extra loading needed to reload the data that is currently being cached, though doing this might also need the cache itself to be cleared as well. It's hard to say without having a complete picture of the engine's behavior here.

This issue is indirectly mentioned here: #72 (comment)

But deserves its own issue.