Memory not being freed, causing giant memory leak

Answer 1

Where's the destructor of ustring? Shouldn't it free the allocated memory?

---

Let's look e.g. at the assignment operator:

ustring operator=(ustring input)
{
    ustring result(input);
    ...
    return *this;
}

You pass a ustring parameter by value. This will likely result in a copy being created via the copy constructor. You invoke copy construction another time to initialize result. I suspect that you invoke copy construction yet another time when you return *this as a ustring (again by value, instead of by reference).

Let's look at one of these three cases, namely result: When this local variable goes out of scope (ie. at the end of the function), it will be automatically destroyed. But if you don't provide a destructor (~ustring) that frees the allocated memory, you'll get a memory leak.

And since you apparently have lots of copy construction and pass-by-value going on without having a destructor for freeing the allocated memory, you'll get lots and lots and lots of unfreed memory.

---

Besides: Why do you use malloc and free instead of new[] and delete[]? You could get rid of the ugly sizeof(int) * ... calculations and (int*) type casts. Instead of:

values = (int *) malloc(sizeof(int) * len);

you'd simply write:

values = new int[len];

Answer 2

  ustring operator=(ustring input) {
    ustring result(input);
    free(values);
    len = input.len;
    values = input.values;
    return * this;
  }

Why is result being declared in this?

Answer 3

1. Try using RAII-techniques to avoid problems with memory leaks. If you wrap your arrays in something like a boost::shared_array you will not have to write an explicit destructor to free the memory.
2. ustring operator=(ustring input) should be ustring operator=(const ustring & input) to avoid a copy of the argument. This should always be done if you pass a non-integral type as read-only parameter.
3. Using shared_array are something like it also gets rid of the problem of calculating size.

Just some hints. On another note, your code is incredibly hard to read and understand. You should really do something about that if you expect other persons to ever work with that. Make your names more descriptive, refactor recurring code into functions that express what it does. For instance instead of using a for-loop to copy your values, you could use std::copy. You may also consider to replace all your magic numbers (e.g. 0x1f) by constants whose names express the semantics of the value.

Answer 4

As soon as You implement the destructor, this implementation of assignment will bite You

  ustring operator=(const char * input) {
    ustring result(input);
    free(values);
    len = result.len;
    values = result.values;
    return * this;
  }

No, it is not an argument for leaving the destructor not implemented.

EDIT:

ustring object owns an array allocated with malloc and should free that memory when it gets destroyed.

If you create a local object

    ustring result(input);

It will get destroyed when the function returns.

In the line

    values = result.values;

You copy the pointer from Your local object - result, but result doesn't know about it, so it should destroy the array and leave *this with a dangling pointer. You have to change the state of result in a way, that prevents it from freeing the memory, because You took the ownership of the array from it. One way to do that could be setting it's pointer to null. Calling free on a null pointer is legal, so You don't have to worry about an attempt to free memory by result's destructor.

ustring& operator=(const char * input) {
    ustring result(input);
    free(values);
    len = result.len;
    values = result.values;
    result.values = 0;
    return * this;
}

Answer 5

Basically, you create far too many ustrings, you need a LOT more references, and you didn't implement a destructor so when they all fall off the stack, they don't get freed.

Also, when in your assignment operator, you need to set result.values to NULL, else the memory will be deleted. You could use a move operator to make this a nice fast operation, although I still don't understand why you would.