4

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 !

rocambille
  • 15,398
  • 12
  • 50
  • 68
roy.atlas
  • 411
  • 2
  • 8

4 Answers4

3

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.

Community
  • 1
  • 1
rocambille
  • 15,398
  • 12
  • 50
  • 68
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.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • Not only assignment operator should be implemented, but also copy constructor and move operations (cf. rule of 3/5/0), unless the idea of manually managing memory is dropped for using `std::string`. Also the error is not caused by `b = a;` but by `b.name = "abc";` (but without `b.name = "abc";`, `b = a;` would indeed have caused an error) – rocambille Dec 22 '16 at 16:55
  • @wasthishelpful First of all you are entirely wrong. The reason of the error is the absence of the copy assignment operator. Secondly I pointed out in my answer that the copy constructor must be defined explicitly. And thirdly there is no need to define move special functions for such simple program. – Vlad from Moscow Dec 22 '16 at 17:00
  • My apologies, you indeed mentionned the copy constructor ^^ We don't know the real usage behind the OP, so IMHO we can't say there is no need to define move special functions. We mentionned them here, I'm happy enough ^^ I disagree with you: the deleted memory in the code of the OP is **not** the pointer copied from `a`. It would have been without reassignment of `b.name` and then I would have agreed with you on the cause of the error in the OP. Beyond the OP, the missing copy assignment operator is indeed an error, so I agree with you anyway :) – rocambille Dec 22 '16 at 17:13
0

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)

Simon Elliott
  • 2,087
  • 4
  • 30
  • 39
0

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.

Paul Evans
  • 27,315
  • 3
  • 37
  • 54