Problem with malloc and free

[sheldon]

Hey,

during the use of malloc and free I encountered an error in my function, but I was not able to localise it.

char setPlay::m_setNameOfPlayer(int getNumberOfPlayer)
{
	int c,d;
	
	c = 0;
	d = 0;

	printf("Name of Player:");
	printf("\n\nEntry: ");
	scanf("%s",&NameOfPlayer);	
	
	
		for(c = 0; c<=10; c++)
		{
			if(NameOfPlayer[c] != '\0')
					d++;
			else
				c = 11;			
		}
		
		/*d = d+1;*/
		Buffer = (char *)malloc(sizeof(char)*(d));
		for(c = 0; c<=d;c++)
			Buffer[c] = 0;
		
		for(c = 0; c<=d; c++)
			Buffer[c] = NameOfPlayer[c];	
		
	return *Buffer;
	
}

Theoretically this function and

setPlay::~setPlay()
{
	free(Buffer);	
}

the destructor should work fine, but instead I get an error about a "bad pointer". During the debugging I was able to detect, that after leaving the function "m_setNameOfPlayer(int getNumberOfPlayer)" the variable "char Buffer" loses its content and causes the error.
With this in mind I am still not able to figger out what causes the error.
It appears at run time, building the project works fine.

"Buffer" is defined in a header file. Like everything else, which has to be there.

Anyone a suggestion?

[Disch]

1) Any particular reason you like malloc/free over new/delete? If you're using C++ you should really get in the habit of using new/delete instead.

2) You can avoid this problem if you use strings instead of char arrays.

3) I'm a little unclear as to what the difference between 'NameOfPlayer' and 'Buffer' is.

4) m_setNameOfPlayer only returns the first character in the name of the player, not the whole string.

5) I smell buffer overflows here. How big is NameOfPlayer? You're not allocating enough space for buffer to contain the null terminator, etc. Again, using std::string avoids all these problems.

6) your dtor frees the buffer correctly -- so it should be working unless you create an instance of the setPlay class which doesn't call m_setNameOfPlayer. If m_setNameOfPlayer is never called, the buffer is never allocated, and thusly you try to free a bad pointer. (assuming you don't have a ctor / your ctor doesn't initialize 'Buffer')

Example:

{
  setPlay foo;
  foo.m_setNameOfPlayer(4);  // allocates Buffer
}  // frees Buffer

{
  setPlay bar;
}  // frees Buffer (but it was never allocated!) 

7) if m_setNameOfPlayer is called more than once, you have memory leaks because you don't free all the buffers you allocated.

I don't know whether or not #6 is your problem. It's hard to tell from the code you posted. It's possible you have some heap corruption due to buffer overflows or a misunderstanding of char pointers.

[jrohde]

An additional thing I noticed, is that your for loop is quite strange. If you want to break out of the loop, just use the 'break' statement - you don't need to set the value of 'c' to 11.

[Disch]

you also could just use strlen:

d = strlen(NameOfPlayer);

and strcpy to copy the string, etc.

[sheldon]

Thanks for a lot of suggestions and tipps!

I am strongly influenced by someone who writes his C++ with a lot of C elements, I took over a few habits of his ;)
Herefore I avoided to use the string lib.
#4 was also in my mind, through tests I found nothing wrong.

I created an instance of m_setNameOfPlayer in this function, sorry, forgot to post it.

void setPlay::m_constructGame()
{
	setPlay *p_setPlay = new setPlay;
	NumberOfPlayer = p_setPlay->m_setTypeOfGame();
	p_setPlay->m_setNameOfPlayer(NumberOfPlayer);
	p_setPlay->~setPlay();

}

I tried to construct my own "versions" of "strlen()" etc. because of that I used this loops.

NameOfPlayer depends on the entry of the user.

@jrohde Thanks for the hint, didn´t thought about it.

[Disch]

I am strongly influenced by someone who writes his C++ with a lot of C elements

I used to be a C/C++ mishmash coder. Partly the reason I clung to C concepts was because I didn't fully understand C++ and OOP constructs. I can't say that the C++ way is "better" all around, but I find that once you get it, it really does make most things a lot easier. It takes a completely different mindset though -- one your mentor might not fully have.

Herefore I avoided to use the string lib.

Don't avoid it. It's better. Less error prone, easier syntax, and will have better performance most of the time.

There are instances where you're better off not using std::string, but for simple string tasks like what you're doing, there's really no reason not to use it.

I tried to construct my own "versions" of "strlen()" etc. because of that I used this loops.

Why? Are you doing this just for educational purposes / to see if you can do it?

As for your code snippit, you have a few problems:

1) Don't explicitly call dtors like that. The only time you should explicitly call a dtor is if you're overloading your own delete operator.

2) Always make sure to delete whatever you make with new

3) Is the m_constructGame a static function? If not, there's no need to create a new setPlay instance in the function like that. You can just use 'this'.

I get the impression you don't fully grasp the concept of classes yet. Can you post your entire setPlay class? Maybe I can point out what you should and shouldn't be doing.

[sheldon]

It takes a completely different mindset though -- one your mentor might not fully have.

He is a professional programmer and for a long time in business. I just started with C/C++ and therefore don´t have much experience, for as you saw, I made a lot of mistakes and forgot about easier ways.

Why? Are you doing this just for educational purposes / to see if you can do it?

Exactly the point. I was just, in a manner of speaking, playing around with the code, collect experience.

I get the impression you don't fully grasp the concept of classes yet

Likely. I read about the stuff, but theory and practical knowledge are not the same ;)

1) Don't explicitly call dtors like that. The only time you should explicitly call a dtor is if you're overloading your own delete operator.

'Will have it in mind next time ;)

3) Is the m_constructGame a static function? If not, there's no need to create a new setPlay instance in the function like that. You can just use 'this'.

Right, another point I have forgotten.

I think posting the entire class is not necessary, because this programm was just a little practise.
By means of your support I learned a lot and will try to rewrite my idea in a more prettier way.
So far, thank you very much for your support!

[Disch]

He is a professional programmer and for a long time in business.

I didn't mean to sound like I doubt his skills/qualifications. Not at all. =)

[sheldon]

No offence taken ;) Didn´t want to engender the impression, that you could have meant something like that. Never thought you would doubt it, therefore don´t worry :)