Creating a move assignment function, keep getting "pointer being freed was not allocated"

Question

I am trying to create a move assignment function but I keep get getting "pointer being freed was not allocated"

const MyString& MyString::operator=(MyString&& move){
    cout<< "Move Assignment" << endl;
    if(this != &move){
        delete[] str;
        len = move.len;
        for(int i = 0; i<len;i++){
            str[i] = move.str[i];
        }
        move.str=nullptr;
        move.len = 0;
        return *this;
    }

    return *this;
}

a.out(37068,0x1000b45c0) malloc: * error for object 0x1001025a0: pointer being freed was not allocated a.out(37068,0x1000b45c0) malloc: * set a breakpoint in malloc_error_break to debug

Please create a [mcve]. There are several big problems with your function, but to be able to give a complete answer, we would need to see more of how `MyString` is implemented. — alter_igel, Jun 10 '19 at 22:03
This is not really moving. You are coping data (to somewhere not existing...). Actually, you'd just want to set a new pointer. — Aconcagua, Jun 10 '19 at 22:06
If you pass by value and take advantage of the [Copy and Swap Idiom](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) self assignment becomes impossible AND you can use the same operator for move and regular assignment. — user4581301, Jun 10 '19 at 22:57

Nikos C. · Accepted Answer · 2019-06-10T22:23:57.180

This:

delete[] str;

deletes str. But then:

str[i] = move.str[i];

str is deleted. So this is undefined behavior.

Anyway, this is not how to do a move. The whole point of a move is to avoid copying the string. Assuming str is a char*, then a correct implementation would be the following (the common name for the argument is rhs, meaning "right hand side"):

MyString& MyString::operator=(MyString&& rhs) noexcept
{
    std::swap(len, rhs.len);
    std::swap(str, rhs.str);
    return *this;
}

Again: this is only a correct implementation if str is just a pointer. Note that the implementation doesn't delete anything. The deletion will happen in the destructor of rhs. The noexcept specifier is not required, but since this implementation can never throw an exception, marking it noexcept allows for some more optimizations by the compiler.

Thanks. Looks like it worked. And I did have another problem with my program but you also answered it by fixing my move assignment. Thanks! — Dhruv Patel, Jun 11 '19 at 04:20

Aconcagua · Answer 2 · 2019-06-11T05:20:19.867

Addition to Nikos C.'s answer:

Swapping is not the only solution – but it is quite an elegant one: You retain the memory of the target string to be reused in the source string. While fine so far, you might want to re-start with an empty string after having moved. Again you shouldn't delete the memory, it suites nicely for being re-used. So you'd just set the length to 0.

However, then you'd need to remember separately, how many characters yet fit into memory. But this is useful anyway. Think about, would you want to re-allocate the string's memory every time you append a single character?

Most likely not. So you'd add some additional memory (e. g. doubling the capacity, if you run out of memory). All put together:

class MyString
{
    size_t length;
    size_t capacity;
    char* data;
public:
    MyString& operator=(MyString&& other)
    {
        if(&other != this)
        {
            std::swap(data, other.data);         // as in Nikos' answer
            std::swap(capacity, other.capacity);
            length = other.length;
            other.length = 0;                    // restart with empty string
                                                 // still you have quite a bit of
                                                 // memory already reserved
        }
        return *this;
    }
};

Be aware that this is optional, though, and actually, you possibly make people pay for something they might not need – if they don't reuse the moved from object...

The only thing you are allowed to do with moved-from objects is to either assign something new to them, or destroy them. So setting the length to 0 isn't actually needed. It even hurts here, because you had to re-introduce the self-assignment guard. — Nikos C., Jun 11 '19 at 04:33
@NikosC. Actually, it's *exactly* the same what `std::string` does (GCC implementation, at least) – apart from not setting first byte to 0. So if you are right, GCC has a bug... Any reference to the standard? — Aconcagua, Jun 11 '19 at 05:05
Turns out, it's up to each type to define what the moved-from state is. For library types, see [this answer](https://stackoverflow.com/a/12095473/856199). The "valid but unspecified state" option is a good choice for custom types too. The string implementation in libstdc++ might have other reasons to do this. Maybe it's because of the short string optimization, I don't know. In any event, it's not a bug. It's just that it's not needed in this case. — Nikos C., Jun 11 '19 at 05:10
In one respect, you are right, though: This implementation violates the principle "don't pay for what you don't need" – *if* you want to reuse the object, then `clear` it explicitly... — Aconcagua, Jun 11 '19 at 05:13

Creating a move assignment function, keep getting "pointer being freed was not allocated"

2 Answers2