free(): double free detected in tcache 2 on calling overloaded assignment operator

Question

I'm working on a university project where we are to implement some of the c++ string class as Mystring. I'm working on the overloaded assignment operator and this is the current code for it:

Mystring& Mystring::operator=(const Mystring& orig)
{
    if(this != &orig)
    {
        delete ptr_buffer;
        len = orig.len;
        buf_size = orig.buf_size;
        ptr_buffer = orig.ptr_buffer;
    }
    return *this;

}

ptr_buffer, len. and buf_size are the three private variables for the Mystring class. This is my main program to test:

void check (const Mystring s, const string name)
{
    cout << "checking " << name << endl;
    cout << name << " contains " << s << endl;
    cout << name << " capacity() is " << s.capacity() << endl;
    cout << name << " length() is " << s.length() << endl;
    cout << name << " size() is " << s.size() << endl;
    cout << name << " max_size() is " << s.max_size() << endl << endl;
}


int main()
{
    Mystring s1("Hi there!");
    check(s1, "s1");

    Mystring s2("Testing before assignment!");
    check(s2, "s2");
    s2 = s1;
    check(s2, "s2");
    return 0;
}

This is what that outputs:

checking s1
s1 contains Hi there!
s1 capacity() is 10
s1 length() is 9
s1 size() is 9
s1 max_size() is 1073741820

checking s2
s2 contains Testing before assignment!
s2 capacity() is 27
s2 length() is 26
s2 size() is 26
s2 max_size() is 1073741820

checking s2
s2 contains Hi there!
s2 capacity() is 10
s2 length() is 9
s2 size() is 9
s2 max_size() is 1073741820

free(): double free detected in tcache 2

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

As you can see, the assignment DOES work to set all the member variables, however I get a non-zero exit code and a free(): double free detected in tcache 2 error. What am I doing wrong and what does this error mean? I've confirmed that the exit code is coming from calling the assignment, because when I comment out s2 = s1;, it completes with exit code 0.

`delete ptr_buffer;` should be `delete[] ptr_buffer;` if you use `new[]` — Ted Lyngmo, Oct 22 '21 at 14:33
`orig.ptr_buffer` is still set. When `orig` is destroyed, presumably its destructor deletes it — , Oct 22 '21 at 14:34
Hint: After `s2 = s1`, where do their respective `ptr_buffer` fields point to? What happens if you destroy one or the other? — Botje, Oct 22 '21 at 14:34
You have 2 pointers to the same buffer after copy. You need to `ptr_buffer = new char[buf_size];` and copy the data from `orig.ptr_buffer` — Ted Lyngmo, Oct 22 '21 at 14:35
@Frank Thanks, I can't believe I missed that. I changed ptr_buffer = orig.ptr_buffer to strcpy(ptr_buffer, orig.ptr_buffer) and that worked. I also changed delete to delete[] — Drake Ford, Oct 22 '21 at 14:40
@DrakeFord No, that doesn't work. It will _only_ work if you have space in the buffer. You need to `new[]` a new buffer first - but think about this: You only need to `delete[]` the buffer if it doesn't have `capacity` for the `size` in `orig`. If you already have the capacity, don't `delete[]`, just copy from `orig` - if you need to `delete[]` you also need to `new[orig.buf_size]` - and don't forget to adjust the `capacity` accordingly. — Ted Lyngmo, Oct 22 '21 at 14:40
@DrakeFord You should not (ever, really) use `strcpy()`. `std::memcpy()` does the same job in a much safer way, and in your case, faster too since you know the length of the string already. — , Oct 22 '21 at 14:40

Vlad from Moscow · Accepted Answer · 2021-10-22T14:55:17.503

In this copy assignment operator

Mystring& Mystring::operator=(const Mystring& orig)
{
    if(this != &orig)
    {
        delete ptr_buffer;
        len = orig.len;
        buf_size = orig.buf_size;
        ptr_buffer = orig.ptr_buffer;
    }
    return *this;
}

there are at least two problems. If the array of characters pointed to by the pointer ptr_buffer was allocated dynamically then you have to use the operator delete [] instead of delete

delete [] ptr_buffer;

The second problem is that after this assignment

ptr_buffer = orig.ptr_buffer;

two pointers point to the same dynamically allocated memory.

You need to allocate a new extent memory and copy there the string of the assigned object.

The operator can be defined at least the following way

Mystring & Mystring::operator =( const Mystring &orig )
{
    if(this != &orig)
    {
        if ( buf_size != orig.buf_size )
        {
            delete [] ptr_buffer;
            ptr_buffer = new char[orig.buf_size];
            buf_size = orig.buf_size;
        } 
        len = orig.len;
        strcpy( ptr_buffer, orig.ptr_buffer );
        // or if the class does not store strings then
        // memcpy( ptr_buffer, orig.ptr_buffer, len ); 
    }

    return *this;
}

nitpick: the comment should read "does not store **null-terminated** strings". The standard `std::string`, allows for nulls in the middle of the string just fine. — , Oct 22 '21 at 14:59

score 2 · Answer 2 · 2021-10-22T14:53:36.913

Presumably, Mystring has a destructor that looks like:

Mystring::~Mystring() {
  delete[] ptr_buffer;
}

So, you cannot just grab ownership of ptr_buffer from orig without replacing it with something else. That's feasible in a move-assignment, but since you are in a copy-assignment, you must leave orig as is.

This means you have to copy orig's data into it, allocating a new one if necessary:

Mystring& Mystring::operator=(const Mystring& orig)
{
    if(this != &orig)
    {
        std::size_t min_buf_size = orig.len + 1; // or perhaps orig.buf_size, it depends.

        // no need to allocate a new buffer if the current one is big enough already.
        if(buf_size < min_buf_size) {
          delete[] ptr_buffer;
          buf_size = min_buf_size;
          ptr_buffer = new char[buf_size];
        }

        len = orig.len;

        assert(buf_size >= len + 1);
        std::memcpy(ptr_buffer, orig.ptr_buffer, len + 1);
    }
    return *this;
}

free(): double free detected in tcache 2 on calling overloaded assignment operator

2 Answers2