Does this use of "new Mystring" create a memory leak? C++

Question

Before anyone start saying that I should use std::string, this is an exercise part of a course, and I'm trying to learn here, not just to make something work.

The code runs ok, but I'm trying to understand when memory leaks occur, and when not. And how to check for that!!

Mystring Mystring::operator+(const Mystring rhs) {
    Mystring *temp = new Mystring { *this };
    int a = strcat_s(temp->str, strlen(str) + strlen(rhs.str)+1, rhs.str);
    return (*temp);
}

I know I'm using a *temp pointer there that was created with new Mystring and I'm aware I'm not using delete for that new (and I would not know where to put it, by the way...)

But, the code runs ok with no errors, is there a way to check for memory leaks? Is this leaking? Why? Or if not, why not?

Here is the code for everything, the problematic part is at the very end:

#include <iostream>
#include "Mystring.h"

using namespace std;

int main() {
      
    Mystring s1 {"FRANK"};
  
    s1 = s1 + "*****";
    cout << s1 << endl;               // FRANK*****       
    
    return 0;
}

#ifndef _MYSTRING_H_
#define _MYSTRING_H_

class Mystring
{
    friend std::ostream& operator<<(std::ostream& os, const Mystring& rhs);
    friend std::istream& operator>>(std::istream& in, Mystring& rhs);

private:
    char* str;      // pointer to a char[] that holds a C-style string
public:
    Mystring();                                                         // No-args constructor
    Mystring(const char* s);                                     // Overloaded constructor
    Mystring(const Mystring& source);                    // Copy constructor
    Mystring(Mystring&& source);                         // Move constructor
    ~Mystring();                                                     // Destructor

    Mystring& operator=(const Mystring& rhs);     // Copy assignment
    Mystring& operator=(Mystring&& rhs);           // Move assignment

    Mystring operator+(const Mystring rhs);

    void display() const;

    int get_length() const;                                      // getters
    const char* get_str() const;
};

#endif // _MYSTRING_H_

#include <iostream>
#include <cstring>
#include "Mystring.h"

 // No-args constructor
Mystring::Mystring() 
    : str{nullptr} {
    str = new char[1];
    *str = '\0';
}

// Overloaded constructor
Mystring::Mystring(const char *s) 
    : str {nullptr} {
        if (s==nullptr) {
            str = new char[1];
            *str = '\0';
        } else {
            str = new char[strlen(s)+1];
            //strcpy(str, s);
            strcpy_s(str, strlen(s)+1, s);
        }
        std::cout << "Overloaded constructor used" << std::endl;
}

// Copy constructor
Mystring::Mystring(const Mystring &source) 
    : str{nullptr} {
        str = new char[strlen(source.str)+ 1];
        //strcpy(str, source.str);
        strcpy_s(str, strlen(source.str) + 1, source.str);
        std::cout << "Copy constructor used" << std::endl;
}

// Move constructor
Mystring::Mystring( Mystring &&source) 
    :str(source.str) {
        source.str = nullptr;
        std::cout << "Move constructor used" << std::endl;
}

 // Destructor
Mystring::~Mystring() {
    delete [] str;
}

 // Copy assignment
Mystring &Mystring::operator=(const Mystring &rhs) {
    std::cout << "Using copy assignment" << std::endl;

    if (this == &rhs) 
        return *this;
    delete [] str;
    str = new char[strlen(rhs.str) + 1];
    //strcpy(str, rhs.str);
    strcpy_s(str, strlen(rhs.str) + 1, rhs.str);
    return *this;
}

// Move assignment
Mystring &Mystring::operator=( Mystring &&rhs) {
    std::cout << "Using move assignment" << std::endl;
    if (this == &rhs) 
        return *this;
    delete [] str;
    str = rhs.str;
    rhs.str = nullptr;
    return *this;
}

// Display method
void Mystring::display() const {
    std::cout << str << " : " << get_length() << std::endl;
}

// getters
int Mystring::get_length() const { return strlen(str); }
const char *Mystring::get_str() const { return str; }

// overloaded insertion operator
std::ostream &operator<<(std::ostream &os, const Mystring &rhs) {
    os << rhs.str;
    return os;
}

// overloaded extraction operator
std::istream &operator>>(std::istream &in, Mystring &rhs) {
    char *buff = new char[1000];
    in >> buff;
    rhs = Mystring{buff};
    delete [] buff;
    return in;
}

Mystring Mystring::operator+(const Mystring rhs) {
    Mystring temp{ *this };
    int a = strcat_s(temp.str, strlen(str) + strlen(rhs.str) + 1, rhs.str);
    return (temp);
}

I tried to do this without new using:

Mystring Mystring::operator+(const Mystring rhs) {
    Mystring temp { *this };
    int a = strcat_s(temp.str, strlen(str) + strlen(rhs.str)+1, rhs.str);
    return (temp);
}

But that code throws an error when executing the destructors at the end:

HEAP CORRUPTION DETECTED

Why is that other code NOT working? I'm expecting the return function to return the Mystring temp by copy and then delete that temp by going out of scope, but somehow there is a move happening instead of a copy and a str pointer from temp ends up in the result object (s1), which causes the error when the destructor for s1 is called.

Is there a way of doing this without new?

Answer 1

Your use of new in operator+ indeed creates a memory leak, because you are not delete'ing the new'd object before return'ing a copy of it. Every new needs a matching delete, and every new[] needs a matching delete[].

You are on the right track by removing new from your operator+ code.

The problem is, when you create the temp object as a copy of *this (whether you use new or not), that constructor allocates its str member only large enough to hold a copy of the str value being copied from. When you then concatenate the rhs.str value into temp.str (which is not large enough to also hold a copy of rhs.str), you end up writing beyond the bounds of temp.str into surrounding memory, corrupting that memory.

You misused the destsz parameter of strcat_s(), by giving it your desired temp.str size rather than the actual temp.str size, so it couldn't protect you from this mistake.

To do what you are attempting, you would need something more like this instead:

Mystring Mystring::operator+(const Mystring &rhs) const {
    Mystring temp;
    delete[] temp.str; // to avoid leaking what your default constructor new's
    int len = strlen(str) + strlen(rhs.str) + 1;
    temp.str = new char[len];
    strcpy_s(temp.str, len, str);
    strcat_s(temp.str, len, rhs.str);
    return temp;
}

Another option would be to give Mystring another constructor that lets the caller specify the initial capacity of the str member:

Mystring::Mystring(int initialCapacity) 
    : str{nullptr} {
    str = new char[initialCapacity + 1];
    *str = '\0';
}

Mystring Mystring::operator+(const Mystring &rhs) const {
    int len = strlen(str) + strlen(rhs.str);
    Mystring temp{ len };
    strcpy_s(temp.str, len+1, str);
    strcat_s(temp.str, len+1, rhs.str);
    return *temp;
}

Alternatively, implement a separate operator+= for Mystring, and then have operator+ use operator+= to append rhs to temp:

Mystring& Mystring::operator+=(const Mystring &rhs) {
    int len = strlen(str) + strlen(rhs.str) + 1;
    char *newStr = new char[len];
    strcpy_s(newStr, len, str);
    strcat_s(newStr, len, rhs.str);
    delete[] str;
    str = newStr;
    return *this;
}

Mystring Mystring::operator+(const Mystring &rhs) const {
    Mystring temp{ *this };
    temp += rhs;
    return temp;
}