Avoiding memory leak

Question

So I was learning OOP in C++ and I thought it would be a good practice to write my own string class (for learning purposes, of course). I came up with a problem which I didn't know how to solve. Here's some peace of code:

class String {
    char *str;
public:
    String(char const *str);
    ~String();
    String operator + (char const *str);
};

String::String(char *str) {
    this->str = _strdup(str);
}

String::~String() {
    free(this->str);
}

String String::operator+(char const *str) {
    char *temp = (char *) malloc(strlen(str) + strlen(this->str) + 1);
    strcpy(temp, this->str);
    strcat(temp, str);
    return temp;
}

The problem here is, that this piece of code will cause a memory leak. Return from "operator +" invokes my constructor, which copies temp by allocating more memory and I couldn't find any way how can I free it.

You should be following the Rule of Three/Five. Also, prefer `new[]` and `delete[]` to `malloc` and `free`. — chris, Mar 20 '13 at 22:27
Not to mention a whole lotta NULL parameter checks and return values that are outright unaccounted for. — WhozCraig, Mar 20 '13 at 22:29
Writing your own string class is a nice thing for learning purposes... but writing a good string class is not a simple task. — David Rodríguez - dribeas, Mar 20 '13 at 22:30
At first glance, you have more problems than you realize. Just for example, your `operator+` leaks memory -- it allocates space, then returns a String constructed from data in that space, but never frees the what it allocated. — Jerry Coffin, Mar 20 '13 at 22:33

score 7 · Accepted Answer · answered Mar 20 '13 at 22:33

7

Your operator + is defined as returning a String but you're returning a char* which means the compiler is implicitly converting it using the constructor. This copies the string but doesn't free the original which you are therefore leaking.

There are lots of things you could do to improve the code, as others have suggested, but to fix the actual leak you could do this:

String String::operator+(char const *str) {
    char *temp = (char *) malloc(strlen(str) + strlen(this->str) + 1);
    strcpy(temp, this->str);
    strcat(temp, str);
    String strTmp(temp);
    free(temp);
    return strTmp;
}

answered Mar 20 '13 at 22:33

Jonathan Potter

36,172
4
64
79

+1 I was in the middle of writing pretty much the exact same answer – Lorkenpeist Mar 20 '13 at 22:36
Note for future visitors that this answer assumes that you have written a copy constructor & copy assignment operator. Otherwise you just swap a leak for a double `free()` and crash. – mythagel Mar 20 '13 at 22:54

David Rodríguez - dribeas · Answer 2 · 2013-03-20T22:39:09.540

Writing a string class is not a simple task, if you want to do it right. For the particular problem that you are facing I can make a couple of recommandations...

Implement append() or operator+= that creates a larger buffer copies the contents, swaps the internal buffer and the newly created and releases the old one.

Then operator+ becomes a trivial task:

String operator+(String lhs, String const & rhs) {
   lhs += rhs;                 // lhs.append(rhs);
   return lhs;
}

(Of course, this is assuming that you provide correct definitions of the copy constructor, and assignment operator)

score 1 · Answer 3 · answered Mar 20 '13 at 22:31

1

You forgot to implement operator= and the copy constructor. If you don't provide your own, the compiler will implement them for you doing a member wise copy which causes your memory leak.

answered Mar 20 '13 at 22:31

jbruni

1,238
8
12

I didn't, I just didn't put the code here. If it's neccessary -- tell me, I'll add it. – Arnas Mar 20 '13 at 22:32
1

@Arnas, Well, better safe than sorry. You wouldn't believe the number of duplicate questions on here where that is the problem. – chris Mar 20 '13 at 22:33

Avoiding memory leak

3 Answers3

Linked