Thread: Corrupting memory

Code:

if(temps[0] == 'l' && temps[1] == 'o' && temps[2] == 'o' && temps[3] == 'k')
	{

		int number = 0;
		int fix = 0;
		char searchstr[30],tempstr[35];
		char lookmsg[100];
		for(int y=0;y<30;y++)
		{
			if(y == 0 && temps[y+5] - '0' < 10)	//if we are using a number to define the creature
			{
				fix = 2;
			}
			if(temps[y+5] == ' ')				//if we are done
			{
				searchstr[y-fix] = '\0';
				break;
			}
			else								//if not next character in
			{
				searchstr[y-fix] = temps[y+5];
			}
		}
		strcpy(tempstr," ");
		strcat(tempstr,searchstr);
		strcpy(searchstr,tempstr);
		if(0 <= (temps[5] -'0') && (temps[5] -'0') <= 9) //is it a number?
		{
			for(int x=0;x<LNPCS;x++)
			{
				if(npcs[x].IsHere() == Players[ID].IsHere() && strcmp(npcs[x].GetName(),searchstr) == 0)
				{
					number++;
					if(number == (temps[5] -'0'))
					{
						npcs[x].MakeLookMsg(lookmsg);
						cons[ID].send((unsigned char)strlen("CHAT"));
						cons[ID].send("CHAT");
						cons[ID].send((unsigned char)strlen(lookmsg));
						cons[ID].send(lookmsg);							
					}
				}
			}
		}
		else
		{
			for(int x=0;x<LNPCS;x++)
			{
				if(npcs[x].IsHere() == Players[ID].IsHere() && strcmp(npcs[x].GetName(),searchstr) == 0)
				{
					npcs[x].MakeLookMsg(lookmsg);
					cons[ID].send((unsigned char)strlen("CHAT"));
					cons[ID].send("CHAT");
					cons[ID].send((unsigned char)strlen(lookmsg));
					cons[ID].send(lookmsg);				
				}
			}
		}
	}else

Error message at runtime is:
Stack around the variable 'tempstr' was corrupted.

I figure it is occuring when I try to add a leading space to the search string. I don't understand what is making me overstep the allocated memory. Not sure if using pointers will help the situation any either. Anyone have some advice for me?

edit:
LNPCS is the current number of npcs in the game forgot I had that in...

All <cstring> functions like strcpy() and strcat() only operate on null terminated char arrays, and these:

char searchstr[30],tempstr[35];

start off filled with junk values. In addition, when you use strcat() you have to have enough room in the destination array. The room left in the destination array is the room from the first null termination character to the end of the array. That has to be big enough to fit the source array from index position 0 to to the first null termination character(inclusive).

To build on what 7stud said, copying a string from a 35-char buffer to a 30-char buffer is just asking for trouble.

Originally Posted by CornedBee

To build on what 7stud said, copying a string from a 35-char buffer to a 30-char buffer is just asking for trouble.

Actually I started with two 30's figured I was overrunning the space in one to cause that error.... might go look into the null char. Mighta thought I had it in another part thanks for the help. Gonna go look now.

edit:

worked a bit more put some outputs in so I can log what contains what info. Here is the new code more comments some other things were changed nothing big.

Code:

if(temps[0] == 'l' && temps[1] == 'o' && temps[2] == 'o' && temps[3] == 'k')
	{
		log<<"Look was triggered"<<endl;
		log<<"temps = "<<temps<<endl;
		int number = 0;
		int fix = 0;
		char searchstr[30],tempstr[35];//changed code further down this is safe now.
		char lookmsg[100];
		for(int y=0;y<30;y++)
		{
			if(y == 0 && temps[y+5] - '0' < 10)	//if we are using a number to define the creature
			{									//ex 2.Goblin
				fix = 2;						//need to move the chars 2 to the left.
			}
			if(temps[y+5] == ' ')				//if we are done
			{
				searchstr[y-fix] = '\0';		//NULL char...
				break;							//kick out of the loop
			}
			else								//if not next character in
			{
				searchstr[y-fix] = temps[y+5];
			}
		}
		log<<"Search String = "<<searchstr<<endl;
		strcpy(tempstr," \0");					//is this better than " "?
		strcat(tempstr,searchstr);				//so I should have " Goblin" or what have you.
		log<<"tempstr = \""<<tempstr<<"\""<<endl;
		if(fix == 2)							//is it a number?  Found this is faster to check
		{
			for(int x=0;x<LNPCS;x++)			//loop through all the NPCS in the game
			{
				if(npcs[x].IsHere() == Players[ID].IsHere() && strcmp(npcs[x].GetName(),tempstr) == 0)
				{//If the NPC is in the same spot as the player AND its name == what I sent.
					number++;
					log<<"Found a NPC with a matching name and is in correct spot."<<endl;
					if(fix == 2 && number == (temps[5] -'0'))//ok weird... dunno what I was thinkin
					{
						npcs[x].MakeLookMsg(lookmsg);
						cons[ID].send((unsigned char)strlen("CHAT"));
						cons[ID].send("CHAT");
						cons[ID].send((unsigned char)strlen(lookmsg));
						cons[ID].send(lookmsg);							
					}
				}
			}
		}
		else
		{
			for(int x=0;x<LNPCS;x++)
			{
				if(npcs[x].IsHere() == Players[ID].IsHere() && strcmp(npcs[x].GetName(),tempstr) == 0)
				{
					npcs[x].MakeLookMsg(lookmsg);
					cons[ID].send((unsigned char)strlen("CHAT"));
					cons[ID].send("CHAT");
					cons[ID].send((unsigned char)strlen(lookmsg));
					cons[ID].send(lookmsg);				
				}
			}
		}
	}else

This is the relevant log file.

Code:

COMMAND: look Goblin
Look was triggered
temps = look Goblin
Search String = Goblin
tempstr = " Goblin"

OK so I just now noticed the line it was giving me wasn't even in this whole if it is at the end of the function so does that mean it is having trouble freeing the memory or something?

> if(temps[0] == 'l' && temps[1] == 'o' && temps[2] == 'o' && temps[3] == 'k')
Try using
if ( strncmp( temps, "look", 4 ) == 0 )

> if(fix == 2)
You also compare the same variable for the same value inside this condition, and since there is no obvious sign of fix changing, the inner test is useless.

Already, this is looking like part of a much larger function. Split it up into more manageable parts.
> for(int x=0;x<LNPCS;x++)
You have two very similar looking blocks of code. Extract a parameter or two, and make the rest into a separate function.

> for(int y=0;y<30;y++)
It's possible for this loop to exit without ever appending a \0

> strcpy(tempstr," \0");
1. tempstr[0] = '\0'; is better
2. Given that this is C++, why not use all the comfort and safety of C++ strings rather than the awkward and error prone C-style char arrays?

Originally Posted by Salem

> if(temps[0] == 'l' && temps[1] == 'o' && temps[2] == 'o' && temps[3] == 'k')
Try using
if ( strncmp( temps, "look", 4 ) == 0 )

Didn't know about that function but will use in the future

> if(fix == 2)
You also compare the same variable for the same value inside this condition, and since there is no obvious sign of fix changing, the inner test is useless.

Yea... I got rid of that before I fell asleep last night.

Already, this is looking like part of a much larger function. Split it up into more manageable parts.

It is a large function called translate. It determines what the character is doing by interperating the data they send.

> for(int x=0;x<LNPCS;x++)
You have two very similar looking blocks of code. Extract a parameter or two, and make the rest into a separate function.

could try that... I am looking into it.

> for(int y=0;y<30;y++)
It's possible for this loop to exit without ever appending a \0

I think that is my problem... but still when I toss it into a file I don't see the extra junk that I usually see when I leave a '\0' off of a string.

> strcpy(tempstr," \0");
1. tempstr[0] = '\0'; is better
2. Given that this is C++, why not use all the comfort and safety of C++ strings rather than the awkward and error prone C-style char arrays?

IDK char arrays seem to come more natural to me as odd as that sounds. Also I need that leading whitespace.