Crash upon delete through destructor

Question

In the following program, i intend to copy char* line contents from one object to another through strcpy. However when the program ends, destructor of obj2 works fine but dtor of obj crashes. gdb shows different addresses of line for both objects.

class MyClass {
        public:
                char *line;
                MyClass() {
                        line = 0;
                }
                MyClass(const char *s) {
                        line = new char[strlen(s)+1];
                        strcpy(line, s);
                }
                ~MyClass() {
                        delete[] line;
                        line = 0;
                }
                MyClass &operator=(const MyClass &other) {
                        delete[] line;
                        line = new char[other.len()+1];
                        strcpy(line, other.line);
                        return *this;
                }
                int len(void) const {return strlen(line);}
};

int main() {
        MyClass obj("obj");
        MyClass obj2 = obj;

Even though you use C-style null-terminated strings you're still programming in C++. — Some programmer dude, Nov 25 '19 at 15:07
You need a copy constructor too. [Rule of three](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) — ChrisMM, Nov 25 '19 at 15:08
That is because i was asked to simulate copy string in c++ through strcpy — anurag86, Nov 25 '19 at 15:09
Just as an aside, once you add a copy constructor: `MyClass obj1; MyClass obj2 = obj1;` will still segfault because you will call `strlen(obj1.line)` which is `strlen(NULL)`. As would `MyClass obj1; obj1.len();`. — Bill Lynch, Nov 25 '19 at 15:12
Also undefined behavior: `MyClass obj1; obj1.len();` It is undefined behavior to call `strlen` on a null pointer. — PaulMcKenzie, Nov 25 '19 at 15:23

score 15 · Accepted Answer · answered Nov 25 '19 at 15:10

With

MyClass obj2 = obj;

you don't have assignment, you have copy-construction. And you don't follow the rules of three, five or zero since you don't have a copy-constructor, so the default-generated one will just copy the pointer.

That means after this you have two object whose line pointer are both pointing to the exact same memory. That will lead to undefined behavior once one of the objects is destructed as it leaves the other with an invalid pointer.

The naive solution is to add a copy-constructor which does a deep-copy of the string itself, similarly to what your assignment operator is doing.

A better solution would be to use std::string instead for your strings, and follow the rule of zero.

sweenish · Answer 2 · 2019-11-25T16:05:36.367

You need create a copy constructor. This has to do the rule of 3/5. You are creating obj2, which means a copy constructor is invoked, not the copy assignment operator.

Because you don't have a copy constructor, a "shallow" copy is made. This means that line is copied by value. Since it's a pointer, both obj and obj2 are pointing to the same memory. The first destructor gets called and erases that memory just fine. The second constructor gets called and a double delete occurs, causing your segmentation fault.

class MyClass {
public:
  char *line = nullptr;
  std::size_t size_ = 0;  // Need to know the size at all times, can't 
                          // rely on null character existing
  const std::size_t MAX_SIZE = 256;  // Arbitrarily chosen value
  MyClass() { }
  MyClass(const char *s) : size_(strlen(s)) {
    if (size_ > MAX_SIZE) size_ = MAX_SIZE;
    line = new char[size_];
    strncpy(line, s, size_ - 1);  // 'n' versions are better
    line[size_ - 1] = '\0';
  }
  MyClass(const MyClass& other) : size_(other.size_) {  // Copy constructor
    line = new char[size_ + 1];
    strncpy(line, other.line, size_);
    line[size_] = '\0';
  }
  ~MyClass() {
    delete[] line;
    line = nullptr;
  }
  MyClass& operator=(const MyClass &other) {
    if (line == other.line) return *this;  // Self-assignment guard
    size_ = other.size_;
    delete[] line;
    line = new char[other.size_ + 1];
    strncpy(line, other.line, size_);
    line[size_] = '\0';
    return *this;
  }
  int len(void) const { return size_; }
};

When dealing with C-Strings, you absolutely cannot lose the null character. The issue is that it's extremely easy to lose. You were also lacking a self-assignment guard in your copy assignment operator. That could have led to you accidentally nuking an object. I added a size_ member and used strncpy() instead of strcpy() because being able to specify a maximum number of characters is incredibly important in the case of losing a null character. It won't prevent damage, but it will mitigate it.

There's some other stuff that I did like using Default Member Initialization(as of C++11) and using a constructor member initialization list. A lot of this becomes unnecessary if you are able to use std::string. C++ can be "C with classes" but it's worth taking the time to really explore what the language has to offer.

Something that a working copy constructor and destructor allows us to do is simplify our copy assignment operator using the "copy and swap idiom."

#include <utility>

MyClass& operator=(MyClass tmp) { // Copy by value now
  std::swap(*this, tmp);
  return *this;
}

Link to explanation.

A better solution would be an implementation using copy and swap idiom. — The Philomath, Nov 25 '19 at 15:55

Crash upon delete through destructor

2 Answers2