The destructor is unable to delete a continuous memory block that has been allocated

Question

The following code compiles but delivers an error during run-time:

# include <iostream>
# include <string.h>

class A {
public:
  A() {}
  A ( int id, char * t_name ) {
    _id = id ;
    name = new char [ strlen (t_name) + 1 ] ;
    strcpy ( name, t_name ) ;
  }

  char *name ;
  int _id ;
  ~A() { delete []  name ;}
} ;

int main () {
  A a ( 1, "123" ) ;
  A b ;
  b = a ;

  std::cout << static_cast < const void * > ( a.name ) << std::endl ;
  std::cout << static_cast < const void * > ( b.name ) << std::endl ;

  b.name = "abc" ; // b.name is directed to a different memory block 
  std::cout << static_cast < const void * > ( a.name ) << std::endl ;
  std::cout << static_cast < const void * > ( b.name ) << std::endl ;
  std::cout << a.name << std::endl ;
  std::cout << b.name << std::endl ;

  return 0 ;
}

It outputs something like:

0x7ff87bc03200
0x7ff87bc03200
0x7ff87bc03200
0x10f9bcf64
123
abc
a.out(883,0x7fff7ee3d000) malloc: *** error for object 0x10f9bcf64:
pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

I do not understand why it says:

0x10f9bcf64: pointer being freed was not allocated

since b.name is obviously directed to 0x10f9bcf64, and does not overlap with a's any more!

I also wonder how this issue could be addressed? Thank you !

Answer 1

For starters the constructor declaration should look like

A ( int id, const char * t_name )
            ^^^^^^

because you are using string literals to initialize objects of the class and string literals have types of constant arrays.

The default copy assignment operator makes member-wise copies of data members of objects. Relative to your code after this statement

b = a ;

the objects will have two pointers pointing to the same dynamically allocated memory. Thus the delete operator will be called twice for the same memory address.

You have to write explicitly the copy assignment operator and the copy constructor for your class.

For example the copy assignment operator can look the following way

A & operator = ( const A &a )
{
    if ( this != &a )
    {
        char *tmp = new char[ std::strlen( a.name ) + 1 ];
        std::strcpy( tmp, a.name );

        delete [] this->name;

        this->_id = a._id;
        this->name = tmp;
    }

    return *this;
} 

This statement

b.name = "abc"

is also wrong. String literals have static storage duration. So you may not delete their memory.

Answer 2

You're copying the pointer from instance a to instance b.

When the destructor of instance a runs, it deletes the memory.

When the destructor of instance b runs, it deletes the same memory again.

You need to add a copy and assignment constructor to this class. (And a move constructor if you're using c++11)

Answer 3

I do not understand why it says "0x10f9bcf64: pointer being freed was not allocated" since b.name is obviously directed to 0x10f9bcf64 and does not overlap with a's any more!

Because b's destructor is calling delete [] on a static string.

I also wonder how this issue could be addressed.

You should have defined a copy constructor, something like:

A::A(const A& lhs) {
    _id = lhs.id;
    name = new char [ strlen (lhs.name) + 1 ] ;
    strcpy ( name, lhs.name ) ;
}

And also made name and _id private.

Answer 4

You should first read about Rule of 3/5/0. Your statement:

b = a;

is a violation of the rule of 3 (5 if you're using modern C++, i.e. C++11 or later), since your class A has a pointer as a member.

Next, if your consider this statement:

b.name = "abc";

You're affecting here a static char array you didn't allocate with new. So when your destructor is trying to delete it:

~A() { delete []  name ;}

The call to delete[] generates your error.

A simple solution would be to declare name as a std::string:

class A {
public:
  A () {}
  A ( int id, const std::string& t_name ) {
    _id = id ;
    name = t_name;
  }

  std::string name ;
  int _id ;
} ;

int main () {
  A a ( 1, "123" ) ;
  A b ;
  b = a ;

  std::cout << static_cast < const void * > ( &a.name ) << std::endl ;
  std::cout << static_cast < const void * > ( &b.name ) << std::endl ;

  b.name = "abc" ; // b.name is directed to a different memory block 
  std::cout << static_cast < const void * > ( &a.name ) << std::endl ;
  std::cout << static_cast < const void * > ( &b.name ) << std::endl ;
  std::cout << a.name << std::endl ;
  std::cout << b.name << std::endl ;

  return 0 ;
}

Since std::string manages memory for you, you're back to the wonderful world of rule of 0.