Assignment operator overloading - deleting heap memory causes crash

Question

I am writing to write a string class and while doing assignment operator overloading I am observing crash at the part where we do deletion of previously allocated memory. I tried to trace through code but couldn't figure it out. Any pointers would help

str& str::operator=(const str &Rhs)
{
    if (this != &Rhs)
    {
        cout << " attempt of self allocation - " << endl;
        **delete[] this->_element;**   // crashes here

        this->_capacity = Rhs._capacity;
        this->_display = Rhs._display;
        this->_size = Rhs._size;


        if (Rhs._size > 0)
        {
            this->_element = new char(this->_size);
            if (this->_element == NULL)
            {
                cout << " mem allocation failed " << endl;
            }
        }

        for (int counter = 0; counter <= this->_size; counter++)
        {
            this->_element[counter] = Rhs._element[counter];
        }
    }

    return *this;
}
/*copy constructor */
str::str(const str& Rhs)
{
    // copy constructor called 
    this->_capacity = Rhs._capacity;
    this->_display = Rhs._display;
    this->_size = Rhs._size;

    if (Rhs._size > 0)
    {
        this->_element = new char(_size);
        if (this->_element == NULL)
        {
            cout << " mem allocation failed " << endl;
        }

        for (int counter = 0; counter <= this->_size; counter++)
        {
            this->_element[counter] = Rhs._element[counter];
        }
    }
}

/* constructor */

str::str(const char *Y)
{
    cout << "constructor called !!! -- " <<  endl;
    size_t len = this->stringlen(Y);
    this->_element = new char(len + 1);
    for (int counter = 0; counter < len; counter++)
    {
        this->_element[counter] = Y[counter];
    }
    this->_element[len] = '\0';

    this->_size = len + 1;

    cout << "string in constructor is -- " << this->_element << endl;
}

From .h file

class str 
{
public:
    /*Default Constructor*/
    explicit str();

    /*Constructor with single char Argument*/
    explicit str(char x);

    /*Constructor with char array Argument*/
    explicit str(const char* Y);

    /* Creating new element with copy constructor */
    str(const str& Rhs);

    /* Overloading of Assignment operator */ 
    str& operator=(const str& Rhs);

    friend int string_compare(const str& Lhs, const str& Rhs);
    int reverse();
    size_t stringlen(const char* Y);
    str& operator+(str& Rhs);
    bool operator==(const str& Rhs);
    bool operator!=(const str& Rhs);
    friend ostream& operator<<(ostream &out, str& Lhs);


private:
char*  _element;
int _capacity;
bool _display;
int _size; //largest accessed + 1
};

Main Routine -

void test1() {
  const char *j = "abc";
  cout << "length of j = " << strlen(j) << endl;
  str s1('U');
  str s2("hello");
  cout << s2 << endl;
  s2.reverse();
  cout << s2 << endl;
  str s3(s2);
  cout << s1 << endl;
  cout << s2 << endl;
  cout << s3 << endl;
  **s2 = s1;**  // crashes
  cout << s2 << endl;
  cout << s1 << endl;

}

You do not clear the str properly (delete and set all members to an initial state). You may consider the copy-swap-ideom: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom — , Oct 13 '15 at 18:59
@zenith : I see s2's value before crash to be correct - Here is what I see visual studio "_element 0x00580bd0 "olleh" char *" — thedreamer, Oct 13 '15 at 19:09
Possible duplicate of [Definitive List of Common Reasons for Segmentation Faults](http://stackoverflow.com/questions/33047452/definitive-list-of-common-reasons-for-segmentation-faults) — CodeMouse92, Oct 13 '15 at 19:21
You have a UB because your loop condition should read `counter < this->_size` (not `<=`). Plus it would make sense to set `this->_element` to nullptr after deletion to avoid potential double-delete. To have a definitive answer you need to provide your copy constructor too. — Rostislav, Oct 13 '15 at 19:24
Also, if your `capacity_` member means what I think it means, then you either set it to incorrect value or allocate a incorrectly-sized buffer depending on your logic. — Rostislav, Oct 13 '15 at 19:26
@Rostislav : I am not utilizing capacity member variable - its redundant in code - plan to use it once I get base working - I have added copy constructor part of code - Let me look into for loop which you have pointed to .. — thedreamer, Oct 13 '15 at 19:32
@DieterLücking : Thanks for the great link - I would try to implement the suggested version once I figure out issue here. Curious to debug this further to understand root cause of issue — thedreamer, Oct 13 '15 at 19:33
You have the same problem in copy constructor - you write past the end of allocated array. Given your comment in the header file, you should allocate `_size + 1` elements. Also, you never initialize your `_element` array if the `rhs` has zero size. This will crash eventually too. — Rostislav, Oct 13 '15 at 19:38
@Rostislav - What I have done is in constructor - I have incremented size by 1 - So when copy constructor is called and it assigns size to new object it would be having additional 1 by default - Not sure if it's best practice but do you see any logical problems in the small code which I have ..I would take care of Rhs zero comment (thanks for the pointer) — thedreamer, Oct 13 '15 at 19:47

score 1 · Accepted Answer · answered Oct 13 '15 at 19:43

There are several problems with your code.

The biggest one is: if you want to allocate an array you need to use new char[_size]. new char(_size) allocates one char with its value set to _size.
Second, once you fix that problem, you write past the end of your allocated array - judging by the comment in your header you need to allocate char[_size + 1].
Third, in your copy constructor you never initialize the _element array and in your assignment operator you never clear up the _element value. This will eventually lead to crash when you copy or assign an empty str and then try to assign to it (or at destruction time as I assume that the destructor also calls delete).

The biggest one was the main culprit causing the crash - I do see all your points as vaild too - Thanks for the help !!! — thedreamer, Oct 13 '15 at 19:57

Assignment operator overloading - deleting heap memory causes crash

1 Answers1