problem with destructor

[bArT]

Hi!
I have a problem with such situation. I have a class like below and have
some pointers (ptr1 and ptr2). I dynamically allocate memory in constructor
and I free in destructor. For one pointer there is some error when
dustroctor delete objects. Why? What should I have to change?

Please help.

bart

class A
{
char *ptr1;
char *ptr2;
A(char *string1, char *string2)
{
if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}
virtual ~A()
{
if (ptr1 != NULL)
delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;
}
// other methods. I use there ptr1 and ptr2
}

main()
{

A MyClass("text1", "text2")
return 0;
}

 

[Gernot Frisch]

Hi!
I have a problem with such situation. I have a class like below and have
some pointers (ptr1 and ptr2). I dynamically allocate memory in constructor
and I free in destructor. For one pointer there is some error when
dustroctor delete objects. Why? What should I have to change?

Please help.

bart

class A
{
char *ptr1;
char *ptr2;

A() {ptr1=NULL; ptr2=NULL;}

A(char *string1, char *string2)
{

ptr1=NULL; ptr2=NULL; // because string1 or string2 might be
NULL??

if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}
virtual ~A()
{
if (ptr1 != NULL)
delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;

ptr1=NULL; ptr2=NULL; // Always do this after delete[] - at
least in debug versions.

}
// other methods. I use there ptr1 and ptr2
}

main()
{

A MyClass("text1", "text2")
return 0;
}

HTH,
Gernot

 

[Andre Kostur]

You should first tell us the error....

A() {ptr1=NULL; ptr2=NULL;}

One should prefer the initializer lists to explicit assignment. Doesn't
make much of a different for pointers, but matters more for rich classes.
Better to get into the habit:

A() : ptr1(NULL), ptr2(NULL) {};

ptr1=NULL; ptr2=NULL; // because string1 or string2 might be
NULL??

Same as above:

A(char * string1, char * string2) : ptr1(NULL), ptr2(NULL)

if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}
virtual ~A()
{
if (ptr1 != NULL)

This test isn't necessary. Performing a delete or delete[] on a NULL
pointer is always safe (it does nothing, but it's safe).

delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;

ptr1=NULL; ptr2=NULL; // Always do this after delete[] - at
least in debug versions.

}
// other methods. I use there ptr1 and ptr2
}

main()

int main()

There are no implicit return types in C++.

Offhand, did _this_ specific program cause a failure? If not, post the
shortest, compilable program that exhibits the problem.

One common error is that somewhere along the line you might be causing
the copy constructor of A to be invoked, and your copy constructor
currently doesn't do the right thing. As a general hint, anytime you're
playing with dynamically allocated memory in a class (or pretty much any
other "dynamic" resource), you are likely going to need to supply your
own Constructor, Copy Constructor, Assignment operator, and Destructor.
(Or perhaps declare them as private and don't supply a body if you
explicitly want to disallow copy construction, for example)

 

[Victor Bazarov]

I have a problem with such situation. I have a class like below and have
some pointers (ptr1 and ptr2). I dynamically allocate memory in constructor
and I free in destructor. For one pointer there is some error when
dustroctor delete objects. Why? What should I have to change?

You need to adhere to the "Rule of Three". Look it up.

Please help.

bart

class A
{
char *ptr1;
char *ptr2;
A(char *string1, char *string2)

In order for your code to work well, 'string1' and 'string2' here
should be 'const char*'.

{
if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}

That is all fine, with one exception: what if 'string1' is NULL?
What value does 'ptr1' get? Same for 'ptr2'. In most cases they
will get garbage in them, which will cause an attempt to delete[]
the memory, which in turn will cause undefined behaviour.

Make sure ptr1 and ptr2 are initialised to 0 before the body:

A(const char* s1, const char* s2) : ptr1(0), ptr2(0)
{
if (s1 ...

}
virtual ~A()

Do you _really_ need it virtual?

{
if (ptr1 != NULL)
delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;

Checking is unnecessary. 'this->' is unnecessary.

}
// other methods. I use there ptr1 and ptr2
}
;


main()

int main()

{

A MyClass("text1", "text2")
return 0;
}

The syntax errors suggest that you typed your code into the message
instead of copying it from your real project. That might simply not
show the place where the problem was. Next time post _real_ code.

Victor

 

[bArT]

Doesn't work. I have an error "DAMAGE: after Normal block ....". This is the
error to the same pointer ptr1. When I click Retry I have cursor in
DBGHEAP.H.
What's wrong? I have never had such problem???

bart

class A
{
char *ptr1;
char *ptr2;

A() {ptr1=NULL; ptr2=NULL;}

A(char *string1, char *string2)
{

ptr1=NULL; ptr2=NULL; // because string1 or string2 might be
NULL??

if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}
virtual ~A()
{
if (ptr1 != NULL)
delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;

ptr1=NULL; ptr2=NULL; // Always do this after delete[] - at
least in debug versions.

}
// other methods. I use there ptr1 and ptr2
}

main()
{

A MyClass("text1", "text2")
return 0;
}

HTH,
Gernot

 

[tom_usenet]

Hi!
I have a problem with such situation. I have a class like below and have
some pointers (ptr1 and ptr2). I dynamically allocate memory in constructor
and I free in destructor. For one pointer there is some error when
dustroctor delete objects. Why? What should I have to change?

Please help.

bart

class A
{
char *ptr1;
char *ptr2;
A(char *string1, char *string2)
{
if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}

The first thing to point out is that any class that provides explicit
resource management for more than one resource is hard to write
correctly - in your case, you're controlling two different char*'s,
which means you have exception safety problems (remember, new can
throw an exception). To get around this (and your other problems),
just use std::string.

Here's the string version:

#include <string>

class A
{
std::string s1;
std::string s2;

A(char const* string1, char const* string2)
:s1(string1 != 0? string1: ""),
s2(string2 != 0? string2: "")
{
}

//other methods
};

And that's it, since the destructor, copy constructor and assignment
operator are all written for you by the compiler.

The moral is: avoid explicit memory management - it's error prone,
makes code verbose and exception unsafe and is a particularly bad idea
for beginners.

Tom

 

[bArT]

Doesn't work.

In order for your code to work well, 'string1' and 'string2' here
should be 'const char*'.

const char* doesn't change anything;

That is all fine, with one exception: what if 'string1' is NULL?
What value does 'ptr1' get? Same for 'ptr2'. In most cases they
will get garbage in them, which will cause an attempt to delete[]
the memory, which in turn will cause undefined behaviour.

There isn't such possibility. All strings in constructor are not NULL.

The syntax errors suggest that you typed your code into the message
instead of copying it from your real project. That might simply not
show the place where the problem was. Next time post _real_ code.

My real class is big and there is a lot of code. Generally i use dynamically
alocated char arrays. I copy there strings or append in methods and simply
in destructor I want to free memory.

bart

 

[John Harrison]

Hi!
I have a problem with such situation. I have a class like below and have
some pointers (ptr1 and ptr2). I dynamically allocate memory in
constructor
and I free in destructor. For one pointer there is some error when
dustroctor delete objects. Why? What should I have to change?

Please help.

bart

class A
{
char *ptr1;
char *ptr2;
A(char *string1, char *string2)
{
if (string1 != NULL)
{
ptr1 = new char[strlen(string1)+1];
strcpy(ptr1, string1);
}
if (string2 != NULL)
{
ptr2 = new char[strlen(string2)+1];
strcpy(ptr2, string2);
}
}
virtual ~A()
{
if (ptr1 != NULL)
delete [] this->ptr1;
if (ptr2 != NULL)
delete [] this->ptr2;
}
// other methods. I use there ptr1 and ptr2
}

main()
{

A MyClass("text1", "text2")
return 0;
}

The code you posted does not compile, but with a few changes it will
compile and run correctly.

If you want help with your specific problem instead of general advice on
how to fix the many issues with your code then I suggest you post the
*real* code.

john

 

[Victor Bazarov]

Doesn't work.

What doesn't work? The code you posted wasn't real to start with,
so I couldn't just take it, compile, and run to see it not work.
I gave you some suggestions that could simply make the non-C++ code
you posted slightly better, at least to the extend that it would
compile. What else did you want?

Now, without seeing _real_, _complete_, _compilable_, _minimal_
code that demonstrates the behaviour you claim your program has,
there is no way to give any kind of advice. OK? We are not mind
readers here.

V

 

[Karl Heinz Buchegger]

Doesn't work.

In order for your code to work well, 'string1' and 'string2' here
should be 'const char*'.

const char* doesn't change anything;

That is all fine, with one exception: what if 'string1' is NULL?
What value does 'ptr1' get? Same for 'ptr2'. In most cases they
will get garbage in them, which will cause an attempt to delete[]
the memory, which in turn will cause undefined behaviour.

There isn't such possibility. All strings in constructor are not NULL.

The syntax errors suggest that you typed your code into the message
instead of copying it from your real project. That might simply not
show the place where the problem was. Next time post _real_ code.

My real class is big and there is a lot of code.

Well. Might be. But otherwise we have no real idea of what might
have goofed up your program. The posted program (when corrected
as suggested) doesn't have any problem, so how should we know
what might be your problem?

Generally i use dynamically
alocated char arrays.

Bad idea.
You now have seen what this might lead into.

I copy there strings or append in methods and simply
in destructor I want to free memory.

bart

Then as a first action add a copy constructor and assignment operator
to your class. You can make them private and don't need to implement
them. Then either the compiler or the linker will tell you
where your problem really is.

class A
{
....

private:
A( const A& Arg ); // not implemented by intention
A& operator=( const A& Arg ); // not implemented by intention
};

If I assume that all your string manipulations through strcat, strcpy
are correct, there are 2 possible errors left

* you copy an A object somewhere unexpectedly. Search the web for
'rule of three' to see some explanations of possible pitfalls
for this case, and why the above suggestion can help you to identify
it

* you corrupted memory somewhere else and just see the symptoms in
class A. That's why a small testprogram which just uses your class
in a standalone environment would be a good idea. It can help to
rule out this problem.

 

[bArT]

OK, here is my class... I don't give the whole implementation of other
methods (because as I noticed it doesn't have an influence at my problem)
but only constructors and destructor.

I have problems with pointers pMessage and pServer.

Please check it and help me.

The code you posted does not compile, but with a few changes it will
compile and run correctly.

If you want help with your specific problem instead of general advice on
how to fix the many issues with your code then I suggest you post the
*real* code.

john

class CSmtp
{
public:
//void Initialize();
CSmtp();
CSmtp(int Port, const char *Server, const char *User, const char *Pass,
const char *Title, const char *Message, const char *Receiver, const char
*Sender);
virtual ~CSmtp();

private:
// BOOL RecvMsg();
//void CheckServerError();
//BOOL SendMsg(char *pMsgToSend);
char * pReceiver;
char * pSender;
char * pTitle;
char * pMessage;
// int CheckAuth();
char * pPassword;
char * pUser;
// BOOL CheckResponse(char *pNumber);
// BOOL AuthPlain();
//BOOL AuthLogin();


int nBufferLength;
//BOOL SetBuffer(int nLength);
//BOOL SetTimeout(int nSeconds);
//void CheckSocketError();
char *Buffer;
SOCKET Sock;
char *pServer;
int nPort;
// void ClearBuffer();
// void CloseConnection();
//BOOL SetConnection();

};


CSmtp::CSmtp()
{
pMessage = NULL;
pServer = NULL;
pUser = NULL;
pPassword = NULL;
pReceiver = NULL;
pSender = NULL;
pTitle = NULL;
Buffer = NULL;
nBufferLength = 0;

}

CSmtp::CSmtp(int Port,const char *Server, const char *User,const char *Pass,
const char *Title, const char *Message, const char *Receiver, const char
*Sender)
{
pMessage = NULL;
pServer = NULL;
pUser = NULL;
pPassword = NULL;
pReceiver = NULL;
pSender = NULL;
pTitle = NULL;

nPort = Port;
if (Server != NULL)
{
pServer = new char[strlen(Server+1)];
strcpy(pServer, Server);
}
Buffer = NULL;
nBufferLength = 0;


if (Message != NULL)
{
pMessage = new char[strlen(Message)+5];
strcpy(pMessage, Message);
}
if (User != NULL)
{
pUser = new char[strlen(User)+1];
strcpy(pUser, User);
}
if (Pass != NULL)
{
pPassword = new char[strlen(Pass)+1];
strcpy(pPassword, Pass);
}
if (Title != NULL)
{
pTitle = new char[strlen(Title)+1];
strcpy(pTitle, Title);
}
if (Receiver != NULL)
{
pReceiver = new char[strlen(Receiver)+1];
strcpy(pReceiver, Receiver);
}
if (Sender != NULL)
{
pSender = new char[strlen(Sender)+1];
strcpy(pSender, Sender);
}
}

CSmtp::~CSmtp()
{
if (pMessage != NULL)
delete [] pMessage;
if (pUser != NULL)
delete [] pUser;
if (pPassword != NULL)
delete [] pPassword;
if (pTitle != NULL)
delete [] pTitle;
if (pReceiver != NULL)
delete [] pReceiver;
if (pSender != NULL)
delete [] pSender;

if (pServer != NULL)
delete [] this->pServer;
if (Buffer != NULL)
delete [] Buffer;

pMessage = NULL;
pServer = NULL;
pUser = NULL;
pPassword = NULL;
pReceiver = NULL;
pSender = NULL;
pTitle = NULL;
Buffer = NULL;
}

 

[John Harrison]

OK, here is my class... I don't give the whole implementation of other
methods (because as I noticed it doesn't have an influence at my problem)
but only constructors and destructor.

I have problems with pointers pMessage and pServer.

Please check it and help me.

Well the first thing to note is that you because it crashes in the
destructor does not mean that the problem is with the destructor (or
constructor). I'd like to know how you've established that the other
methods don't have a bearing because that is the first place I would be
looking.

The only way to be sure that the problem is not with the other methods
would be to remove them entirely and still find that the program crashed.
Have you tried that?

But the first thing to do however it to take Karl's advice and add a
private copy constructor and a private assignment operator. If doing that
causes a compile or link error then you've found your problem. Its very
quick to do and to be honest should have been done already. (I'm assuming
here that the copy constructor and assignment operator aren't two of the
methods that you left out in your post.)

If that doesn't work then I think you are reduced to cutting out whole
sections of code until you can isolate the section that is causing the
problem. Remember that it could be anywhere, just because the code crashes
in the destructor of one class, does not mean that is where the problem is.

Unfortunately there is nothing fatally wrong with the code you've posted
(that I can see). So you are going to have to hunt around or post some
more code.

john

 

[bArT]

When I deleted all methods except constructor and destructor I still have
the same error. So other methods can't cause a problem but only this
destructor or constructor.

I can't add private assigment operator, because I have such error: "function
'class CSmtp &__thiscall CSmtp:perator =(const class CSmtp &)' already has
a body".
What is Your advice?

bart

 

[Andre Kostur]

When I deleted all methods except constructor and destructor I still
have the same error. So other methods can't cause a problem but only
this destructor or constructor.

I can't add private assigment operator, because I have such error:
"function 'class CSmtp &__thiscall CSmtp:perator =(const class CSmtp
&)' already has a body".
What is Your advice?

1) Don't top-post

2) Show us your implementation of operator=, you may have an error in
there....

 

[Karl Heinz Buchegger]

When I deleted all methods except constructor and destructor I still have
the same error. So other methods can't cause a problem but only this
destructor or constructor.

I can't add private assigment operator, because I have such error: "function
'class CSmtp &__thiscall CSmtp:perator =(const class CSmtp &)' already has
a body".
What is Your advice?

check that the operator= has a correct implementation.
If you are unsure, post it.

 

[Karl Heinz Buchegger]

When I deleted all methods except constructor and destructor I still have
the same error. So other methods can't cause a problem but only this
destructor or constructor.

I can't add private assigment operator, because I have such error: "function
'class CSmtp &__thiscall CSmtp:perator =(const class CSmtp &)' already has
a body".
What is Your advice?

The block damage error message looks like the one generated by VC++.
If you are working with VC++, you could also try to insert calls
to AfxCheckMemory() in your code at strategic places and thus
pin down the code position where the damage occours.

 

[David Harmon]

On Thu, 29 Jul 2004 18:33:52 +0200 in comp.lang.c++, "bArT"

There isn't such possibility. All strings in constructor are not NULL.

So then why does your constructor say:
if (string1 != NULL)

If you claim that, then your constructor should instead say
assert(string1 != NULL);

 

[bArT]

Ok, I solved the problem.
I changed all char * into string (as somebody said here) and now everything
works properly.
Thanks everyone for help.

bart

 

[Karl Heinz Buchegger]

Ok, I solved the problem.
I changed all char * into string (as somebody said here) and now everything
works properly.

A strong indication that either
* you somewhere wrote to the character array out of bounds.
* you copied such an object without having a well behaved
copy constructor
* you assigned such an object without having a well behaved
assignment operator