1

I am not able to understand this behavior. I have a class A,

class A
{
public:
int ch;
char *s;
A()
{}
A(char *st):ch(0)
{
    s = new char[10];
    strcpy(s,st);

}
A(const A& rhs):ch(rhs.ch)
{
    s = new char[strlen(rhs.s)+1];
    strcpy(s,rhs.s);
}
const A& operator=(const A& rhs)
{
    char *temp = new char[strlen(rhs.s)+1];
    strcpy(temp,rhs.s);
    delete[] s;
    s=temp;
    ch = rhs.ch;
    return *this;
}
~A()
{
    delete []s;
}
};

Till this point everything goes as expected I am able to test my copy constructor and assignment operator and they are working properly.

Now I created a child class B and I am getting heap corruption error. I am not able to understand, is this the problem related somewhere with class A destructor. ? Below is my class B, 

class B:public A
{

public:
int a;
B():a(0){}
};
JackSparrow
  • 707
  • 2
  • 10
  • 24
  • 3
    You probably never used the default constructor for `A` until it was implicitly used by `B`. The problem is the default constructor leaves `s` uninitialized, so it points to nowhere, and when then the destructor comes in and tries to `delete []` it, you get a crash. – jogojapan Jan 04 '13 at 08:05
  • Does the corruption happen when you call **only** the constructor? – Mark Garcia Jan 04 '13 at 08:05
  • @Mark yeah means when I tried to access B class object – JackSparrow Jan 04 '13 at 08:07
  • @jogojapan Yeah may be this is the case let me check again – JackSparrow Jan 04 '13 at 08:09
  • 1
    Your assignment operator is correct (though you should probably not get into the habit of calling delete until the objects state is finalized (non POD may throw when deleted)). But an easier way to write it is to use the `Copy and Swap idiom`. – Martin York Jan 04 '13 at 08:48
  • See: http://stackoverflow.com/a/255744/14065 – Martin York Jan 04 '13 at 08:55
  • @LokiAstari His assignment operator is correct, since he makes sure that he does everything which can throw before modifying his object (which is the essential issue). – James Kanze Jan 04 '13 at 09:08
  • @LokiAstari thanks loki.. I'll follow that .. :)..! – JackSparrow Jan 04 '13 at 09:16
  • @JamesKanze: He does for this case. But not for the general case (which is why I mentioned habbits). The delete should not be done until after the object is in its final state. If the destructor of any object in the array would have thrown then the we now have an object with an inconsistent state. – Martin York Jan 04 '13 at 18:23
  • @LokiAstari In general, I'd tend to agree with you. But the fact that he has taken care to do the allocation before the `delete` suggests quite strongly to me that he has considered the issues. And while using the swap idiom is relatively simple, the important thing is to do everything which might throw before modifying state. (There are even cases where this rule is more than is needed. As long as the object has a coherent state anytime it might throw, you're probably safe.) – James Kanze Jan 04 '13 at 18:49
  • @JamesKanze: You always have to allocation before delete (otherwise you can't make a copy)! So you can't read anything from that. – Martin York Jan 04 '13 at 18:59
  • Also this is really a 3 phase operation. 1) Allocate and copy (dangerous) 2) swap (safe) 3) Deallocate (potentially dangerous). In this case (3) is not dangerous but in the general case you can not assume that and as such should be performed after (2) because it is step 2 where the object state is changed. If you do (3) before (2) you potentially leave the object in a bad state. – Martin York Jan 04 '13 at 19:02
  • @LokiAstari The usual implementation when I was starting C++ did the delete first: you're deleting from the `lhs`, but copying `rhs`. And a common word of wisdom was to check for self assignment first. It's the most "natural" way of writing the operator; it's just not exception safe. The fact that he has moved the allocation before the delete makes me think that he has given the issue some thought. – James Kanze Jan 05 '13 at 10:42
  • @LokiAstari And your step 3 (deallocation) shouldn't be dangerous. If it is, you must be violating the more general rule of never letting an exception escape from a destructor. – James Kanze Jan 05 '13 at 10:43
  • As a container writer you may not always by the author of what is contained in your container (in the general case). But you can still program to mitigate for bad code. Thus (3) d-allocation must come after (2) the safe creation of a consistent object (you can do it first but you would have catch fix and re-throw to compensate). – Martin York Jan 06 '13 at 15:10

3 Answers3

3

To solve your problem all you need to do is to replace: 

char *s;

with 

std::string s;

Just get rid of the manual memory management through char *, this is precisely the reason C++ provides you with std::string.

What might be the problem?

Your default constructor which does not take any arguments does no dynamic allocation.
If you created the class object through this constructor, Your destructor ends up deleteing a pointer which was not allocated with new and thus resulting in Undefined Behavior.

Alok Save
  • 202,538
  • 53
  • 430
  • 533
3

In the destructor, you delete[] s;, but in the default constructor, you haven't new[]ed s. In fact, you haven't even initialized s.

The default constructor of the base class is called when you instantiate the derived class because you have not initialized the base otherwise (: A(...)). Therefore, you have no idea what you're deleting, or even what you're going to have for breakfast tomorrow, because it's undefined behaviour.

To keep it consistent, new[] s in the default constructor. To save headache, I would suggest something like std::string instead of character pointers.

chris
  • 60,560
  • 13
  • 143
  • 205
3

You default constructor for A does not initialise the member s (a pointer):

A()
{}

Hence when elements are constructed using this constructor, you get a crash when the destructor deletes the uninitialized element:

~A()
{
  delete []s;
}

Class B uses the default constructor for A, and therefore triggers this problem. Avoid it by properly initializing all members in the default constructor:

A() : ch(), s(0)
{ }
jogojapan
  • 68,383
  • 11
  • 101
  • 131