Is there any way to avoid memory leak in the following code?

Question

#include <new>

class Foo {
public:
    int *x;
    mutable size_t count {1};
    Foo() : x {new int}  {}
    Foo(const Foo &rhs) : x {new int} {
        if(++rhs.count > 5) {
            throw runtime_error("");
        }
    }
    ~Foo() {
        delete x;
    }
    Foo &operator=(const Foo &) = delete;
};
int main(int argc, char *argv[]) {
    Foo *p {reinterpret_cast<Foo *>(::operator new (sizeof(Foo) * 5))};
    Foo f;
    for(auto i {0}; i < 5; ++i) {
        try {
            new (p + i) Foo(f);
        }catch(...) {
            for(auto j {0}; j < i; ++j) {    //!
                (p + j)->~Foo();
            }
        }
    }
    ::operator delete (p);
}

Please attention to for(auto j {0}; j < i; ++j) in catch(...) {}.

In this code, I can change the for's condition to j <= i to avoid memory leak. But if p is in a template container, the revision before may cause UB. Because p + i hasn't been constructed completely, destructing it directly will cause undefined behaviour.

Is there any way to avoid it, or it is a responsibility for class designer?

Why you may want to use so contrived code? Why can't you just use `Foo *p=new Foo[5]`, or even better, just an array of `std::unique _ptr`? — LoPiTaL, Nov 17 '18 at 15:00
Where did you learn to write code like this? There is a lot of things that should rarely be used in there, e.g. `reinterpret_cast`, `::operator new`, `::operator delete`, `mutable` and placement new. There probably is UB somewhere in all of this. — , Nov 17 '18 at 15:01
This is highly un-idiomatic and weird C++ code. It is really hard to guess what it is supposed to do. I would suggest scraping it all and using idiomatic human readable C++ instead. The way to avoid memory leaks in idiomatic C++ is to use smart pointers and containers. — n. m. could be an AI, Nov 17 '18 at 15:21

LoPiTaL · Accepted Answer · 2018-11-17T15:37:00.040

If this is some interview question, please tell the interviewer that you don't write code like that, then you get the job.

If this is some homework, please, give your teacher the following link so he can learn something: https://www.aristeia.com/EMC++.html

Finally, answering your question:

int main(int argc, char *argv[]) {
    std::unique_ptr<Foo> p[5];
    Foo f;
    try {
        for (int i=0;i<5;++i) {
            //p[i]=std::make_unique<Foo>(f); //Only for C++14
            p[i]=std::unique_ptr<Foo>(new Foo(f));
        }
    } catch (...) {
        //Nothing, all is done "magically" by unique_ptr
    }
}

Now, actually answering your question and making your code even more contrived, you can use a constructor initializer list try-catch (more here)

class Foo {
public:
    int *x;
    mutable size_t count {1};
    Foo() : x {new int}  {}
    Foo(const Foo &rhs) try: x {new int} {
        if(++rhs.count > 5) {
            throw runtime_error("");
        }
    } catch (...) {
        delete x;
        throw;
    }
    ~Foo() {
        delete x;
    }
    Foo &operator=(const Foo &) = delete;
};

The main being the same as yours.

It would be an option, but since the array length is known at compile time, it is easier to use a classic array. Although it could be improved using `std::array` instead. — LoPiTaL, Nov 17 '18 at 15:30

score 1 · Answer 2 · answered Nov 17 '18 at 15:28

As for the memory leak and undefined behavior:

::operator delete (p); does not destroy the objects you manually created in the allocated storage.
If two exceptions are thrown in the copy constructors, you will try to delete the same object multiple times.
The for loop in the catch block should not leak memory. If a constructor throws it is supposed to be in the uninitialized state afterwards. In other words if new int throws, no space will have been allocated that needs freeing. Your manual exception throwing requires you to assure that the new int allocation is deleted again before the constructor throws.
All of the code you are trying to write in main is basically reinventing what Foo* p = new Foo[5]{f}; would do and the memory managment in the class would be working automatically, even if you are throwing from the constructor if you used std::unique_ptr<int> instead of int*.

score 1 · Answer 3 · answered Nov 17 '18 at 15:29

This has nothing to do with whatever happens or doesn't happen in main(). In the constructor:

    if(++rhs.count > 5) {
        throw runtime_error("");

Because the object never finishes constructing, its destructor never gets called. You cannot destroy something unless it's been constructed first, and you're throwing an exception before the object finishes constructing.

But because a member of the class was constructed with new, and because the destructor does not get called, that's where the memory leak occurs.

The only practical way to avoid leaking memory here is to either manually delete x before throwing this exception, and cleaning up what you allocated; or make x an object that's responsible for cleaning itself up, like a unique_ptr, and its destructor will take care of it, when the exception gets thrown. This would be more in line with the RAII principle.

Also, going deeper in the `delete x` thing, it would be nice to comment on try-catch initializer lists: https://stackoverflow.com/a/160164/3742943 — LoPiTaL, Nov 17 '18 at 15:33

Is there any way to avoid memory leak in the following code?

3 Answers3