PB crashes after typing "meta unload podbot"

Immortal_BLG · **PB crashes after typing "meta unload podbot" -** 07-03-2012

Why?: because when metamod unloads your module, attempts of engine to reference any string in the podbot will produce a segfault. For now metamod protects only console variables and commands from it.

Solution: For the engine functions such as "PRECACHE_*", "SET_MODEL" and "MAKE_STRING" transfer of the string to argument of this functions should look like this:

Code:

282: PRECACHE_SOUND (STRING (ALLOC_STRING ("weapons/xbow_hit1.wav")));
286: m_spriteTexture = PRECACHE_MODEL (STRING (ALLOC_STRING ("sprites/lgtning.spr")));
319: pent->v.target = ALLOC_STRING ("fake");

Note: This is just a examples, you have to do it for all functions (PRECACHE_*, SET_MODEL and MAKE_STRING - maybe this is not a complete list....)

P.S. Sorry for bad english.

KWo · *Last edited by KWo; 10-03-2012 at 15:34..*

If I have already allocated a memory area for precached model, what should I use in SET_MODEL function then? I don't think I need to allocate the memory the second time for the model name... I believe I should store somewhere the pointer to that area of precached model, then I should use this pointer somehow in SET_MODEL function. Can You describe it more clearly? My skill of C++ coding seems to be still really poor comparing to skill of other developers at this forum...

[EDIT]
After compilation I'm getting such errors:
282: PRECACHE_SOUND (STRING (ALLOC_STRING ("weapons/xbow_hit1.wav"))); // waypoint add

1>.\dll.cpp(282) : error C2664: 'int (char *)' : cannot convert parameter 1 from 'const char *' to 'char *'

Can You help with this?

Whistler · **Re: PB crashes after typing "meta unload podbot" -** 10-03-2012

Quote:

Originally Posted by KWo

If I have already allocated a memory area for precached model, what should I use in SET_MODEL function then? I don't think I need to allocate the memory the second time for the model name... I believe I should store somewhere the pointer to that area of precached model, then I should use this pointer somehow in SET_MODEL function. Can You describe it more clearly? My skill of C++ coding seems to be still really poor comparing to skill of other developers at this forum...

[EDIT]
After compilation I'm getting such errors:
282: PRECACHE_SOUND (STRING (ALLOC_STRING ("weapons/xbow_hit1.wav"))); // waypoint add

1>.\dll.cpp(282) : error C2664: 'int (char *)' : cannot convert parameter 1 from 'const char *' to 'char *'

Can You help with this?

try casting the string to (char *)
PRECACHE_SOUND (STRING (ALLOC_STRING ((char *)"weapons/xbow_hit1.wav")))

or (more C++'ish):
PRECACHE_SOUND (STRING (ALLOC_STRING (reinterpret_cast<char *>("weapons/xbow_hit1.wav"))))

Immortal_BLG · **Re: PB crashes after typing "meta unload podbot" -** 10-03-2012

Quote:

Originally Posted by Whistler

try casting the string to (char *)
PRECACHE_SOUND (STRING (ALLOC_STRING ((char *)"weapons/xbow_hit1.wav")))

or (more C++'ish):
PRECACHE_SOUND (STRING (ALLOC_STRING (reinterpret_cast<char *>("weapons/xbow_hit1.wav"))))

You are wrong here as it is STRING macro returns the constant pointer.

Code:

#define STRING(offset) (const char *)(gpGlobals->pStringBase + (int)offset)

Therefore it will be correct to make so:

Code:

PRECACHE_SOUND ((char *)STRING (ALLOC_STRING ("weapons/xbow_hit1.wav")));	// C style
PRECACHE_SOUND (const_cast<char *>(STRING (ALLOC_STRING ("weapons/xbow_hit1.wav"))));	// C++ style

Quote:

Originally Posted by KWo

If I have already allocated a memory area for precached model, what should I use in SET_MODEL function then? I don't think I need to allocate the memory the second time for the model name... I believe I should store somewhere the pointer to that area of precached model, then I should use this pointer somehow in SET_MODEL function. Can You describe it more clearly?

I will try: for example when you call the PRECACHE_MODEL() function you transfer through argument a string (a name of model to precache) which is in address space of your module, the engine sets this pointer to the sv.model_precache array and then uses it, and so, when metamod unloads your module, address space in which was a model name becomes invalid therefore if the engine will try to compare for example your string with any another - at once segfault will jump out.

Code:

// From Q1 sources, but it's still actual....
void PF_precache_model (const char *const s)
{
	int		i;
	
	if (sv.state != ss_loading)
		PR_RunError ("PF_Precache_*: Precache can only be done in spawn functions");

	PR_CheckEmptyString (s);

	for (i=0 ; i<MAX_MODELS ; i++)
	{
		if (!sv.model_precache[i])
		{
			sv.model_precache[i] = s;	// Here the engine takes pointer to model name from address space of your module.
			return;
		}
		if (!strcmp(sv.model_precache[i], s))	// Here we can get segfault if 'sv.model_precache[i]' points into address space of unloaded module.
			return;
	}
	PR_RunError ("PF_precache_model: overflow");
}

The Storm · **Re: PB crashes after typing "meta unload podbot" -** 11-03-2012

I didn't check the macro ALLOC_STRING but in this way of allocation I think that we may end up with memory leak? Will the engine free the string?

Immortal_BLG · **Re: PB crashes after typing "meta unload podbot" -** 11-03-2012

The engine will free this memory in function 'Sys_ShutdownMemory', so leaks won't be

Whistler · **Re: PB crashes after typing "meta unload podbot" -** 11-03-2012

> You are wrong here as it is STRING macro returns the constant pointer.

sorry... I did not checked the actual code before posting

KWo · *Last edited by KWo; 11-03-2012 at 21:27..*

Quote:

Originally Posted by KWo

If I have already allocated a memory area for precached model, what should I use in SET_MODEL function then? I don't think I need to allocate the memory the second time for the model name... I believe I should store somewhere the pointer to that area of precached model, then I should use this pointer somehow in SET_MODEL function.

I think You didn't get what I'm asking for... I was refering to the example with precached lgtning.spr here:
dll.cpp
286: m_spriteTexture = PRECACHE_MODEL ("sprites/lgtning.spr");
then m_spriteTexture (which - I believe - it's a pointer to the model name string) is used i.e. in util.cpp:
1755: WRITE_SHORT (m_spriteTexture);
So - the question was:
a) should SET_MODEL function should be used the same way - with stored pointer:
now in dll.cpp:
291: SET_MODEL (pent, "models/player/urban/urban.mdl");
but maybe - if I want to use this model name couple of times in code - it should be:
m_model_urban = PRECACHE_MODEL(const_cast<char *>(STRING (ALLOC_STRING ("models/player/urban/urban.mdl"))));
SET_MODEL (pent, m_model_urban);
b) if the model in example a) doesn't need to be precached, because it was already precached somewhere, how the function SET_MODEL should look like?
c) if I want to use the model name once to set - should the example look like:
SET_MODEL(const_cast<char *>(STRING (ALLOC_STRING ("models/player/urban/urban.mdl"))))?

For me it's more clear if I can see 2 lines of working examples than somebody writes me 1 page of the theory which I cannot get if I cannot see the result how does it work like.
I mean please write 1 example for each function types You were refering to (in the first post) they need to be changed. I mean 1 example how it should look like each SET_MODEL function, 1 example how it should look like MAKE_STRING etc - with eventually something which needs to be added before (like storing a pointer or something or precaching a string and so on). I know this is a pre-school of C++, but as I said - I know how to make the logic to make some working (and even really complicated) code, but I have lack of C++ basic knowledge, so anything new for me makes me troubles if I don't see the working example.
Thanks to everybody for helping and understanding.

[EDIT]
Finally I did all those ALLOC_STRING for proposed SET_MODEL, PRECACHE_* and MAKE_STRING functions and meta unload finally doesn't crash the server, but there are additionally questions:
a) how to use the pointer to precached model mechgibs in dll.cpp - end of ServerActivate function:
1824: PRECACHE_MODEL((char *)STRING (ALLOC_STRING ("models/mechgibs.mdl")))
later in the code in StartFrame function:
1968: SET_MODEL (pClient->pIllumEnt, (char *)STRING (ALLOC_STRING ("models/mechgibs.mdl")));
I mean - if that second ALLOC_STRING leads or doesn't lead to memory leak because of allocating the memory for the same string couple of times - I can feel here some problem.
b) after succesful unloading podbot_mm.dll all slots used by bots still are in use (also You can see not moving bots on the server - like clients away from the keyboard - how to remove them automatically while using meta unload command?
[/EDIT]

Immortal_BLG · *Last edited by Immortal_BLG; 12-03-2012 at 04:18..*

Quote:

Originally Posted by KWo

dll.cpp
286: m_spriteTexture = PRECACHE_MODEL ("sprites/lgtning.spr");
then m_spriteTexture (which - I believe - it's a pointer to the model name string) is used i.e. in util.cpp:
1755: WRITE_SHORT (m_spriteTexture);

No. m_spriteTexture it's just a model index (can be used in arrays such as: 'sv.models', 'sv.model_precache', 'sv.model_precache_flags', and God knows where else...) AND THIS IS NOT A POINTER!
See example from HL engine: (It was necessary at once and to lay out, I simply was too lazy)

Code:

int PF_precache_model_I (const char *const modelName)
{
	bool v1 (false);

	if (modelName == NULL)
		Host_Error ("PF_precache_model_I: NULL pointer");

	if (PR_IsEmptyString (modelName))
		Host_Error ("PF_precache_model_I: Bad string '%s'", modelName);

	if (modelName[0u] == '!')
	{
		++modelName;	// Skip '!' character.

		v1 = true;
	}

	unsigned int modelIndex (0u);

	if (sv.state == ss_loading)
	{
		// Check if model with given name already precached....
		for (/* Empty */; sv.model_precache[modelIndex] != NULL; ++modelIndex)
		{
			if (Q_stricmp (sv.model_precache[modelIndex], modelName) == 0)
				return modelIndex;	// If we find it, return it's index....

			// If no free slots left, print error and exit....
			if (modelIndex == MAX_MODELS)
				Host_Error
				(
					"PF_precache_model_I: Model '%s' failed to precache because the item count is over the %d limit.\n"
					"Reduce the number of brush models and/or regular models in the map to correct this.",

					modelName, MAX_MODELS
				);
		}

		// register new model
		sv.model_precache[modelIndex] = modelName;
		sv.models[modelIndex] = Mod_ForName (modelName, true, true);

		if (!v1)
			sv.model_precache_flags[modelIndex] |= 1u;
	}
	else
	{
		// Check if model with given name already precached....
		for (/* Empty */; sv.model_precache[modelIndex] != NULL; ++modelIndex)
		{
			if (Q_stricmp (sv.model_precache[modelIndex], modelName) == 0)
				return modelIndex;	// If we find it, return it's index....

			if (modelIndex == MAX_MODELS)	// We can't precache model when server is running, so if we have not found already precached model with given name, print error and exit....
				Host_Error ("PF_precache_model_I: '%s' Precache can only be done in spawn functions", modelName);
		}
	}

	return modelIndex;
}

But I'm talking ONLY about strings, which you pass as arguments in functions (for example see above argument 'modelName') such as 'SET_MODEL', 'PRECACHE_*' and use function ALLOC_STRING instead of MAKE_STRING - that's all.

Quote:

Originally Posted by KWo

So - the question was:
a) should SET_MODEL function should be used the same way - with stored pointer:
now in dll.cpp:
291: SET_MODEL (pent, "models/player/urban/urban.mdl");
but maybe - if I want to use this model name couple of times in code - it should be:
m_model_urban = PRECACHE_MODEL(const_cast<char *>(STRING (ALLOC_STRING ("models/player/urban/urban.mdl"))));
SET_MODEL (pent, m_model_urban);
b) if the model in example a) doesn't need to be precached, because it was already precached somewhere, how the function SET_MODEL should look like?
c) if I want to use the model name once to set - should the example look like:
SET_MODEL(const_cast<char *>(STRING (ALLOC_STRING ("models/player/urban/urban.mdl"))))?

All should look as before, except for a code which is below....
Here is complete list of things that you should change in your code:

Code:

int Spawn (edict_t *pent)
{
282:      PRECACHE_SOUND (const_cast <char *> (STRING (ALLOC_STRING ("weapons/xbow_hit1.wav")))); // waypoint add
283:      PRECACHE_SOUND (const_cast <char *> (STRING (ALLOC_STRING ("weapons/mine_activate.wav")))); // waypoint delete
284:      PRECACHE_SOUND (const_cast <char *> (STRING (ALLOC_STRING ("common/wpn_hudon.wav")))); // path add/delete done
285:      PRECACHE_SOUND (const_cast <char *> (STRING (ALLOC_STRING ("debris/bustglass1.wav")))); // waypoint error found
286:      m_spriteTexture = PRECACHE_MODEL (const_cast <char *> (STRING (ALLOC_STRING ("sprites/lgtning.spr"))));

291:      SET_MODEL (pent, const_cast <char *> (STRING (ALLOC_STRING ("models/player/urban/urban.mdl"))));

299:      SET_MODEL (pent, const_cast <char *> (STRING (ALLOC_STRING ("models/player/terror/terror.mdl"))));

307:      SET_MODEL (pent, const_cast <char *> (STRING (ALLOC_STRING ("models/player/vip/vip.mdl"))));

319:         pent->v.target = ALLOC_STRING ("fake");
320:         pent->v.targetname = ALLOC_STRING ("fake");
}

void ServerActivate (edict_t *pEdictList, int edictCount, int clientMax)
{
1824:   PRECACHE_MODEL("models/mechgibs.mdl");
}

void StartFrame (void)
{
1968:               SET_MODEL (pClient->pIllumEnt, const_cast <char *> (STRING (ALLOC_STRING ("models/mechgibs.mdl")))); // sets it a model
}

Quote:

Originally Posted by KWo

if that second ALLOC_STRING leads or doesn't lead to memory leak because of allocating the memory for the same string couple of times - I can feel here some problem.

For the engine it is not a problem - it does not lead to memory leaks, but it just will reserve more space in memory than necessary. It means that in memory reserved by the engine will store so much identical strings, how many and entities in game. (for example string "models/mechgibs.mdl" - for each bot).

But it can be bypassed easily, in such a way: (just a example, you can write something more pretty)

Code:

enum String_t
{
	String_xbow_hit1,
	String_mine_activate,
	String_wpn_hudon,
	String_bustglass1,
	String_lgtning,
	String_urban,
	String_terror,
	String_vip,
	String_fake,
	String_mechgibs,

	String_Total
};
char *strings[String_Total] =
{
	"weapons/xbow_hit1.wav",
	"weapons/mine_activate.wav",
	"common/wpn_hudon.wav",
	"debris/bustglass1.wav",
	"sprites/lgtning.spr",
	"models/player/urban/urban.mdl",
	"models/player/terror/terror.mdl",
	"models/player/vip/vip.mdl",
	"fake",
	"models/mechgibs.mdl"
};
void GameDLLInit (void)
{
	// ...
	for (unsigned char stringIndex (0u); stringIndex < String_Total; ++stringIndex)
		strings[stringIndex] = const_cast <char *> (STRING (ALLOC_STRING (strings[stringIndex])));
	// ...
}

int Spawn (edict_t *pent)
{
282:      PRECACHE_SOUND (strings[String_xbow_hit1]); // waypoint add
283:      PRECACHE_SOUND (strings[String_mine_activate]); // waypoint delete
284:      PRECACHE_SOUND (strings[String_wpn_hudon]); // path add/delete done
285:      PRECACHE_SOUND (strings[String_bustglass1]); // waypoint error found
286:      m_spriteTexture = PRECACHE_MODEL (strings[String_lgtning]);

291:      SET_MODEL (pent, strings[String_urban]);

299:      SET_MODEL (pent, strings[String_terror]);

307:      SET_MODEL (pent, strings[String_vip]);

319:         pent->v.target = MAKE_STRING (strings[String_fake]);
320:         pent->v.targetname = MAKE_STRING (strings[String_fake]);
}

void ServerActivate (edict_t *pEdictList, int edictCount, int clientMax)
{
1824:   PRECACHE_MODEL(strings[String_mechgibs]);
}

void StartFrame (void)
{
1968:               SET_MODEL (pClient->pIllumEnt, strings[String_mechgibs]); // sets it a model
}

Quote:

Originally Posted by KWo

b) after succesful unloading podbot_mm.dll all slots used by bots still are in use (also You can see not moving bots on the server - like clients away from the keyboard - how to remove them automatically while using meta unload command?

You must first (before calling function BotFreeAllMemory() in Meta_Detach() (line = 206)) kick all the bots:

Code:

PbCmdParser (NULL, g_rgpszPbCmds[PBCMD_REMOVEBOTS], NULL, NULL, NULL, NULL, NULL);

Code:

C_DLLEXPORT int Meta_Detach (PLUG_LOADTIME now, PL_UNLOAD_REASON reason)
{
   // this function is called when metamod unloads the plugin. A basic check is made in order
   // to prevent unloading the plugin if its processing should not be interrupted.

   // is metamod allowed to unload the plugin ?
   if ((now > Plugin_info.unloadable) && (reason != PNL_CMD_FORCED))
   {
      LOG_CONSOLE (PLID, "%s: plugin NOT detaching (can't unload plugin right now)", Plugin_info.name);
      LOG_ERROR (PLID, "%s: plugin NOT detaching (can't unload plugin right now)", Plugin_info.name);
      return (FALSE); // returning FALSE prevents metamod from unloading this plugin
   }

   // Remove all bots from game....
   PbCmdParser (NULL, g_rgpszPbCmds[PBCMD_REMOVEBOTS], NULL, NULL, NULL, NULL, NULL);

   // Delete all allocated Memory
   BotFreeAllMemory();

   return (TRUE); // returning TRUE enables metamod to unload this plugin
}

NOTE: allocation of memory for a classname of the entity isn't necessary for the CREATE_NAMED_ENTITY function

KWo · *Last edited by KWo; 12-03-2012 at 21:01..*

OK. Thanks for the explanation.

I would like to go forward that change about checking by the bots if there is a darkness or not (to prevent use an extra entity with mechigbs.mdl). But until now I didn't get what I need to add in the code, if I need to update my HLSDK for some functions and so on. I'll try to read that topic second time, then I'll probably ask about more explanation.

[EDIT]
Why exactly in ServerActivate - there is no need to allocate the memory for the string?
1824: PRECACHE_MODEL("models/mechgibs.mdl");
Or You just maybe have forgotten to write it so:
1824: PRECACHE_MODEL(const_cast <char *> ALLOC_STRING("models/mechgibs.mdl"));
[/EDIT]