Heap Corruption from Cvar Deconstruction? - PM I'm lookin at you here ;)

c0ldfyr3 · **Author**

Ok I'm using a Macro to initialise all of my cvars so that the admins have a shorter way to access them, I've written a callback to check which one is being used and if it's the shorthand one it calls SetValue on the longer version so regular processing can occur, if it's the longhand one I manually change the member variables to avoid anything firing.

The problem I'm having is evident first by my meta cvars output:
[img]http://img340.**************/img340/200/wtffm.png[/img]
Note the blank cvar at the start, when you see my code I imagine you'll be as dumbstruck as I am right now. CVars seem to be listed in reverse order to the header file which initialises them and I've search my entire codebase for anything that could be creating this strange anomoly.

On server shutdown I get a heap corruption breakpoint for every convar it destroys...
[img]http://img169.**************/img169/7049/wtf2.png[/img]

Which in turn leads to an actual error:
[img]http://img256.**************/img256/67/wtf3u.png[/img]

My code is simple enough...

Code:

#define ShortCvar "zm_"
#define LongCvar "zombie_"
#define CreateConvar( sName, sDefault, lFlags, sHelp, bMin, fMin, bMax, fMax, oCallback ) \
	ConVar zm_##sName(ShortCvar#sName, sDefault, lFlags, sHelp, bMin, fMin, bMax, fMax,oCallback ); \
	ConVar zombie_##sName(LongCvar#sName, sDefault, lFlags, sHelp, bMin, fMin, bMax, fMax, oCallback );
		 
CreateConvar( version, ZOMBIE_VERSION, (FCVAR_REPLICATED | FCVAR_SPONLY | FCVAR_NOTIFY), "ZombieMod Plugin version", false, 0.0f, false, 0.0f, CVar_CallBack );

So from looking at the code, you can see my preprocessor definition can't be creating the empty cvar which I'm fingering for the problem as it has zm_ or zombie_ prepending each one!

Any thoughts on what could be going wrong?

c0ldfyr3

In case it helps I'm appending my Callback functionality, the reason it's so hairy is due to changes since the corruption started happening, I've been trying my hardest to avoid any situation where a ConVar may be created in my callback but to no avail...

PHP Code:


			
char sTmp[100];
char sNewVarName[100];
char sPrefix[8];

// Create string name of the alternate cvar.
if ( lZombieVar == CVAR_ORIGINAL )
{
    Q_strncpy( sPrefix, "zm_", sizeof( sPrefix ) );
    Q_StrRight( sName, Q_strlen( sName ) - 7, sTmp, 100 );
}
else // CVAR_SHORT
{
    Q_strncpy( sPrefix, "zombie_", sizeof( sPrefix ) );
    Q_StrRight( sName, Q_strlen( sName ) - 3, sTmp, 100 );
}
Q_snprintf( sNewVarName, sizeof( sNewVarName ), "%s%s", sPrefix, sTmp );

// Get reference to ConVars.
ConVarRef cRef = ConVarRef( sName );
ConVarRef cNewRef = ConVarRef( sNewVarName );

// Set alternate cvar values.
cNewRef.m_pConVarState->m_fValue = cRef.m_pConVarState->GetFloat();
cNewRef.m_pConVarState->m_nValue = cRef.m_pConVarState->GetInt();

// Set string value...
char sString[5120] = "";
Q_strncpy(sString, (char*)cRef.m_pConVarState->GetString(), 5120 );
if ( Q_strlen( sString ) > 0 )
{
    // Only set this pointer to a value value if the string exists, otherwise maybe it's what's causing the issue as the issue is happening during deleting of this string! Maybe it's because this isn't a string pointer and is being destroyed at the end of my function?? I'm afraid to use SetValue in case it fires the cvar again but this can be hacked.
    Q_strncpy( cNewRef.m_pConVarState->m_pszString, sString, Q_strlen( sString ) + 1 ); 
}

// If it was the short version we want to fire anything that happens during the firing of the long version, I'm still not 100% sure if we need this, the code above (which isn't in this post) could be amended instead.
if ( lZombieVar == CVAR_SHORT )
{
    char sCommand[1024];
    if ( Q_strlen(sNewVarName) == 0 || Q_strlen(sString) == 0 )
    {
        Assert( false ); // Should never happen but...
    }
    Q_snprintf( sCommand, sizeof( sCommand ), "%s \"%s\"\n", sNewVarName, sString ); // Create command.
    m_Engine->ServerCommand( sCommand ); // Could be tidied but useful for now.
}

API

Hmm... I am confused as to why this is happening. CreateConVar (your macro) just creates two ConVar objects. In theory, you should be able to copy and paste the non-macro version of this and produce the same issue. Have you tried dual declarations to manually test?

BAILOPAN

C'mon, you can debug. Look at the stack, it's dying in a destructor. The heap is corrupted. Look for a double free or a memcpy()/memset() that ran amok.

BAILOPAN · *Last edited by BAILOPAN; 10-23-2010 at 22:21.*

Code:

    Q_strncpy( cNewRef.m_pConVarState->m_pszString, sString, Q_strlen( sString ) + 1 );

Is this a memcpy() to some internal pointer whose bounds or allocation size are (at least, given the snippet), are completely indeterminate? ;)

If that's not it, use Valgrind (which would fine this error instantly). Tools are great, they'll save you the time of waiting for crowd-source debugging.

c0ldfyr3

Quote:

Originally Posted by pimpinjuice

CreateConVar (your macro) just creates two ConVar objects. In theory, you should be able to copy and paste the non-macro version of this and produce the same issue. Have you tried dual declarations to manually test?

It turns it out the fault was completely self contained within the callback function ;)

I've said it before and I'll say it again, sometimes at the end of a 12 hour codathon I get frustrated and fire the last thing I was working on onto the boards no matter how stupid it is :p You'll notice in my comments I even said that might possibly be the problem as it's not getting set to a pointer which will be valid once my procedure finishes.

I've amended my code to the below [entirely inside the if ( Q_strlen( sString ) > 0 ) block] and it now is Assert and crash free, yet the singular blank cvar remains at position number 1. I did a quick test and there's a blank cvar at spot #1 for every MM:Source plugin loaded, will you raise a bug or shall I

?

Thanks for being so honest, it made me do some work for a change ;)

c0ld

PHP Code:


			
char sTmp[100];
char sNewVarName[100];
char sPrefix[8];

// Create string name of the alternate cvar.
if ( lZombieVar == CVAR_ORIGINAL )
{
    Q_strncpy( sPrefix, "zm_", sizeof( sPrefix ) );
    Q_StrRight( sName, Q_strlen( sName ) - 7, sTmp, 100 );
}
else // CVAR_SHORT
{
    Q_strncpy( sPrefix, "zombie_", sizeof( sPrefix ) );
    Q_StrRight( sName, Q_strlen( sName ) - 3, sTmp, 100 );
}
Q_snprintf( sNewVarName, sizeof( sNewVarName ), "%s%s", sPrefix, sTmp );

// Get reference to ConVars.
ConVarRef cRef = ConVarRef( sName );
ConVarRef cNewRef = ConVarRef( sNewVarName );

// Set alternate cvar values.
cNewRef.m_pConVarState->m_fValue = cRef.m_pConVarState->GetFloat();
cNewRef.m_pConVarState->m_nValue = cRef.m_pConVarState->GetInt();

// Set string value...
char sString[5120] = "";
Q_strncpy(sString, (char*)cRef.m_pConVarState->GetString(), 5120 );
if ( Q_strlen( sString ) > 0 )
{
    // Effectively a copy of ConVar::InternalSetValue - I could have just hooked the global callback function but not sure which is less intensive so going with the old reliable.
    if ( cNewRef.m_pConVarState->m_pszString )
    {
        delete[] cNewRef.m_pConVarState->m_pszString;
    }
    int lLen = Q_strlen(sString) + 1;
    cNewRef.m_pConVarState->m_pszString    = new char[lLen];
    cNewRef.m_pConVarState->m_StringLength = lLen;
    memcpy( cNewRef.m_pConVarState->m_pszString, sString, lLen ); 
}

// If it was the short version we want to fire anything that happens during the firing of the long version, I'm still not 100% sure if we need this, the code above (which isn't in this post) could be amended instead.
if ( lZombieVar == CVAR_SHORT )
{
    char sCommand[1024];
    if ( Q_strlen(sNewVarName) == 0 || Q_strlen(sString) == 0 )
    {
        Assert( false ); // Should never happen but...
    }
    Q_snprintf( sCommand, sizeof( sCommand ), "%s \"%s\"\n", sNewVarName, sString ); // Create command.
    m_Engine->ServerCommand( sCommand ); // Could be tidied but useful for now.
}

c0ldfyr3

BAIL thanks again for not hand feeding me that fix, a lower man would have gotten annoyed, shouted and highlighted the obviousness in sarcastic wit :p

I have a question of "if this were you and you were deciding" between superceding the global callback or setting the string manually as I've done above to avoid the outside world knowing the variable was changed, which would you chose and if you have the time I'd love to hear why?

I've chosen to set the variable manually as it removes a lot more code that isn't necessary the other way but the drawback is all error handling and avoiding is down to myself.

I'd give my left nut to know someone personally who knows C++ well >.< Sometimes theoretical discussions can be so much more enlightening than a thousand white pages.

BAILOPAN

After a 12-hour codathon it's probably time to take a break ;) But when you come back, using tools and gathering data (this is science) should always be the first step. It's hard to fix bugs when all you can see is the Matrix on your screen.

I don't really understand the problem trying to be solved, or its use case. It sounds like you have two options, hooking something or injecting data into an internal object. Whatever solves your problem in the most robust way without damaging the integrity or efficiency of affected code ;)

c0ldfyr3

Yeah you're probably right ;) I've told you before I generally post in frustration and then look and hope if I can't find the solution someone else will

All I see is the Matrix on my screen anyway, I code in ASM and use a tool to convert it so C++ /sarcasm :p

Well I was just wondering what I pro would do to compare against my own thoughts on the issue, I inject the data into the object as it has way less overhead but I wasn't sure if it was correct as I had to change the objects to public in the SDK to do so (done anyway for other reasons) so I thought it was a trade off.