heap corruption in custom string class

Question

So as I'm creating my own string class using smart pointers (so I can get accustomed to them), and it has mostly been going well, except for the operator+() function.

It keeps crashing the program, and when VS debugs the program, the destructor is throwing an exception. I can't seem to pinpoint why, this is the only function causing the program to crash, even when I remove all of the algorithms, and simply return a mystring object.

Any suggestions?

#include <iostream>
#include <string>
#include <memory>
#include <cstring>

using namespace std;

class mystring {
public:
    mystring() : word(make_unique<char[]>('\0')), len(0) {}
    ~mystring() { cout << "goodbye objects!";}
    mystring(const char *message) : word(make_unique<char[]>(strlen(message) + 1)), len(strlen(message)) {
        for (int i = 0; i < len; i++) word[i] = message[i];
        word[len] = '\0';
    }
    mystring(const mystring &rhs) : word(make_unique<char[]>(rhs.len)), len(rhs.len + 1) {
        for (int i = 0; i < len; i++) word[i] = rhs.word[i];
        word[len] = '\0';
    }
    mystring &operator=(const mystring &rhs) {
        if (this != &rhs) {
            releaseWord();
            word = make_unique<char[]>(rhs.len + 1);
            len = rhs.len;
            for (int i = 0; i < len; i++) word[i] = rhs.word[i];
            word[len] = '\0';
        }
        return *this;
    }

    //what is wrong with this function/what should be changed?
    friend mystring operator+(const mystring& lhs, const mystring& rhs) {
        mystring Result;

        int lhsLength = lhs.len, rhsLength = rhs.len;

        Result.releaseWord();
        Result.word = make_unique<char[]>(lhsLength + rhsLength + 1);
        Result.len = lhsLength + rhsLength;
        for (int i = 0; i < lhsLength; i++) Result.word[i] = lhs.word[i];
        for (int j = lhsLength; j < Result.len; j++) Result.word[j] = rhs.word[j];
        Result.word[Result.len] = '\0';


        return Result;
    }

    friend ostream &operator<<(ostream &os, const mystring &message) {
        return os << message.word.get();
    }
    int size() const {
        return len;
    }
private:
    int len;
    unique_ptr<char[]> word;
    void releaseWord() {
        char *temp = word.release();
        delete[] temp;
    }
};

int main()
{
    mystring word1 = "Darien", word2 = "Miller", word3;

    cout << word1 + word2;//causes heap corruption
    word3 = word1 + word2; //causes heap corruption

    return 0;
}

Answer 1

Your operator+ is not copying characters from rhs correctly, because it is using the wrong index to iterate through rhs.word[]. It should look like this instead:

friend mystring operator+(const mystring& lhs, const mystring& rhs) {
    mystring Result;
    int lhsLength = lhs.len, rhsLength = rhs.len;
    Result.word = make_unique<char[]>(lhsLength + rhsLength + 1);
    Result.len = lhsLength + rhsLength;
    for (int i = 0; i < lhsLength; i++)
        Result.word[i] = lhs.word[i];
    for (int i = lhsLength, j = 0; i < Result.len; i++, j++)
        Result.word[i] = rhs.word[j];
    Result.word[Result.len] = '\0';
    return Result;
} 

An alternative option is to use std::copy_n() instead of a manual loop:

#include <algorithm>

friend mystring operator+(const mystring& lhs, const mystring& rhs) {
    mystring Result;
    Result.word = make_unique<char[]>(lhs.len + rhs.len + 1);
    Result.len = lhs.len + rhs.len;
    std::copy_n(lhs.word.get(), lhs.len, Result.word.get());
    std::copy_n(rhs.word.get(), rhs.len, Result.word.get() + lhs.len);
    Result.word[Result.len] = '\0';
    return Result;
} 

Also, your default and copy constructors are not setting up the members correctly (for instance, your copy constructor is not allocating enough room for a null terminator, and is setting len to include the null terminator).

And, your ReleaseWord method is unnecessary, and you should consider implementing move semantics as well.

Try something more like this:

#include <iostream>
#include <string>
#include <memory>
#include <cstring>
#include <algorithm>

using namespace std;

class mystring {
public:
    mystring() : len(0), word(make_unique<char[]>(1)) {
        word[0] = '\0';
    }

    ~mystring() {
        cout << "goodbye objects!";
    }

    mystring(const char *message) : len(strlen(message)), word(make_unique<char[]>(len + 1)) {
        copy_n(message, len, word.get());
        word[len] = '\0';
    }

    mystring(const mystring &rhs) : len(rhs.len), word(make_unique<char[]>(len + 1)) {
        copy_n(rhs.word.get(), rhs.len, word.get());
        word[len] = '\0';
    }

    mystring(mystring &&rhs) : len(rhs.len), word(std::move(rhs.word)) {
    }

    mystring& operator=(const mystring &rhs) {
        if (this != &rhs) {
            mystring tmp(rhs);
            swap(word, tmp.word);
            swap(len, tmp.len);
        }
        return *this;
    }

    mystring& operator=(mystring &&rhs) {
        swap(word, rhs.word);
        swap(len, rhs.len);
        return *this;
    }

    friend mystring operator+(const mystring& lhs, const mystring& rhs) {
        mystring Result;
        Result.len = lhs.len + rhs.len;
        Result.word = make_unique<char[]>(Result.len + 1);
        std::copy_n(lhs.word.get(), lhs.len, Result.word.get());
        std::copy_n(rhs.word.get(), rhs.len, Result.word.get() + lhs.len);
        Result.word[Result.len] = '\0';
        return Result;
    }

    friend ostream &operator<<(ostream &os, const mystring &message) {
        return os << message.word.get();
    }

    int size() const {
        return len;
    }

private:
    int len;
    unique_ptr<char[]> word;
};

Answer 2

Problem in this line:

for (int j = lhsLength; j < Result.len; j++) Result.word[j] = rhs.word[j];

j is wrong for rhs.word[j];

Should be something like rhs.word[j-lhsLength];

you are busting the array limits