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;
}

In some places (like the copy constructor and the default constructor) you forgot to leave space for the null terminator. Note that make_unique<char[]>('\0') allocates zero bytes because '\0' is just another way of writing 0. — Stack Exchange Broke The Law
– Stack Exchange Broke The Law, Commented Feb 12, 2018 at 5:12
ahhh, ok, that makes perfect sense. Would it be if I did something like this instead? mystring() : len(0), word(make_unique<char[]>(1)) { word[0] = '\0'; } — Darien Miller
– Darien Miller, Commented Feb 12, 2018 at 13:01

Joseph Willcoxson · Accepted Answer · 2018-02-12 05:12:29Z

1

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

answered Feb 12, 2018 at 5:12

Joseph Willcoxson

6,0881 gold badge18 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Joseph Willcoxson Over a year ago

Also, is better if your operator= returns a const mystring&, but that doesn't appear to be your problem.

Remy Lebeau · Accepted Answer · 2018-02-12 16:42:28Z

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;
};

This was very helpful, thank you!! Also, looking through your solution, I've noticed a few parts that were new to me, with this block of code in particular: mystring(mystring &&rhs) : len(rhs.len), word(std::move(rhs.word)) { } What's the purpose of this function, as well as the double "&"? Thanks!
@DarienMiller it is a move constructor, which transfers ownership of existing allocated memory instead of allocating new memory. Read up about "rvalue references" and "move semantics", which were added in C++11 (you are clearly using C++14 or later since you are using std::make_unique(), which was added in C++14).

Collectives™ on Stack Overflow

heap corruption in custom string class

2 Answers 2

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related