Throwing exception from constructor memory leak (handle wrapper c++)

Question

I am trying to write a DriverIterator class to iterate over all the volumes in my comuter.

I understand the next class can cause a memory leak since:

current_ = std::make_unique<Driver>(paths);

can throw an exception (for some reason..) and therefore the construction will not be finished, and as a result destructor will not be invokek and the handle will not be closed properly.

As I undesrtand, once I receive the handle, I should stop constructing. But how can I achieve that? FindFirstVolumeW also provides me data that I need to use before constructor is finished.

DriverIterator.hpp:

class DriverIterator final
{
public:
    explicit DriverIterator();
     ~DriverIterator();

private:
    std::unique_ptr<Driver> current_;
    bool is_empty_;
    HANDLE handle_;

public:
    bool is_empty() const;
    Driver get_current() const;
    void next();

private:
    HANDLE start_find();

public:
    DriverIterator(const DriverIterator&) = delete;
    DriverIterator(DriverIterator&&) = delete;
    DriverIterator& operator=(const DriverIterator&) = delete;
    DriverIterator& operator=(DriverIterator&&) = delete;
};

DriverIterator.cpp:

DriverIterator::DriverIterator():
    handle_(start_find())
{}

DriverIterator::~DriverIterator()
{
    try
    {
        FindVolumeClose(handle_);
    }
    catch (...)
    {

    }
}

HANDLE DriverIterator::start_find()
{
    static constexpr uint32_t MAX_BUFFER_SIZE = 1024;
    wchar_t buffer[MAX_BUFFER_SIZE];

    HANDLE handle = FindFirstVolumeW(buffer, MAX_BUFFER_SIZE);
    if (handle == INVALID_HANDLE_VALUE)
    {
        //throw exception
    }

    wchar_t paths[1024];
    DWORD   res_size;

    if (!GetVolumePathNamesForVolumeNameW(buffer, paths, 1024, &res_size))
    {
        //throw exception
    }

    current_ = std::make_unique<Driver>(paths);

    is_empty_ = false;

    return handle;
}

bool DriverIterator::is_empty() const
{
    return is_empty_;
}

Driver DriverIterator::get_current() const
{
    if (is_empty_)
    {
        //throw exception
    }

    return *current_;
}

void DriverIterator::next()
{
    //code...
}

Are you sure the shared_ptr is the correct way. Since you don't allow copy and assignment, this pointer is never shared. I suspect that unique_ptr is better suited here — Michael Veksler, Jun 06 '20 at 09:31
As for your concern, if Driver class is written correctly, an exception in its destructor should not lead to any ill effects or resource leaks. Since Driver is constructed and maintained through a smart pointer, make_shared or make_unique, there is no potential leak here, unless there is a leak in Driver or in the user of DriverIterator — Michael Veksler, Jun 06 '20 at 09:34
@MichaelVeksler You are right, I should have used unique pointer instead of shared one. However, when I talked about memory leak I was talking about the handle (received from FindFirstVolumeW) that will not be closed properly since the constructor of DriverIterator might throw excpetion while constructing the Driver object through a unique pointer. — Daniel Fridman, Jun 06 '20 at 09:39
Bind your temporary resources to a `unique_ptr` with a custom deleter. — IInspectable, Jun 06 '20 at 09:50
As @IInspectable mentions, you can let a unique_ptr with a custom deleter manage the handle. But as it stands, I see no case where you might have an issue. The Driver object is not allocated in the constructor, so there will not be an exception in the constructor. However, the safest thing is to use RAII objects store any resource (s.t. handles) and free them in any relevant case. — Michael Veksler, Jun 06 '20 at 10:09
I think there is a leak, since he is not closing the handle when throwing the constructor. As @llnspectable is saying you can circumvent this allocating the handle on the heap and using a custom deleter. But why do you need to allocate the driver in a unique pointer. Maybe the easyiest would be to just save the driver as an ordinary member variable? — user3726947, Jun 06 '20 at 10:22
@user3726947 I'm saving a smart pointer to this Driver becuase otherwise the constructor of DriverIterator forces me to initialize it in the initialization list becuase Driver has no default constructor. — Daniel Fridman, Jun 06 '20 at 10:25
I've missed the fact that `start_find` is called in the constructor and its return value is used to allocate the handle, while also initializing the smart pointer. This code is confusing. Usually you either initialize all the fields in function, or you don't touch the fields and return the value. Mixing, where part is initialized inside the function, while the handle is returned (to be used for initialization) is a call for trouble. — Michael Veksler, Jun 06 '20 at 10:41
@MichaelVeksler Can you please look at my new comment? I tried to implement what you suggested — Daniel Fridman, Jun 06 '20 at 11:00
@use I'm not suggesting to allocate heap memory at all. You can use a `unique_ptr` with objects that aren't pointers, like handles. That's one of the main reasons why its design allows for custom deleters. There is no resource leak if the c'tor throws an exception after `current_` holds the `unique_ptr`. C++ guarantees that fully constructed subobjects have their d'tors called when the c'tor throws. Though I cannot really comment on code I cannot see (as is the case with this question). — IInspectable, Jun 06 '20 at 13:32

dewaffled · Answer 1 · 2020-06-06T11:48:40.580

1

When an exception is generated from the constructor then the object destructor is not invoked but the destructor of the already constructed members would be called and the memory allocated for the object would be released.

So, idiomatic C++ way would be to define a RAII wrapper for the handle_ member that would provide access to the underlying resource and correctly release it in the destructor, e.g.:

template <typename CloseFnT, CloseFnT close_fn>
class UniqueHandle {
public:
    UniqueHandle()
        : handle_(INVALID_HANDLE_VALUE)
    {
    }

    UniqueHandle(HANDLE handle)
        : handle_(handle)
    {
    }

    ~UniqueHandle()
    {
        if (handle_ != INVALID_HANDLE_VALUE) {
            close_fn(handle_);
        }
    }

    UniqueHandle(const UniqueHandle&) = delete;
    UniqueHandle& operator = (const UniqueHandle&) = delete;

    UniqueHandle(UniqueHandle&& other)
        : handle_(INVALID_HANDLE_VALUE)
    {
        std::swap(handle_, other.handle_);
    }

    UniqueHandle& operator = (UniqueHandle&& other)
    {
        std::swap(handle_, other.handle_);
        return *this;
    }

    HANDLE get() const {
        return handle_;
    }

private:
    HANDLE handle_;
};

using UniqueVolumeHandle = UniqueHandle<decltype(&FindVolumeClose), FindVolumeClose>;

Then this wrapper can be used everywhere instead of raw HANDLE:

private:
    UniqueVolumeHandle handle_;
...
    handle_ = FindFirstVolumeW(buffer, MAX_BUFFER_SIZE);
    if (handle_.get() == INVALID_HANDLE_VALUE) {
        ... // handle error, throw exception is OK
    }
...
    // use handle
    if (!FindNextVolumeW(handle.get(), buffer, MAX_BUFFER_SIZE)) {
        ... // handle error
    }

// Return wrapped handle
UniqueVolumeHandle start_find() {
    ...
    UniqueVolumeHandle handle = FindFirstVolumeW(buffer, MAX_BUFFER_SIZE);
    ...
    return handle;
}
...

Such approach is implemented for instance in microsoft/wil library.

edited Jun 06 '20 at 11:48

answered Jun 06 '20 at 11:27

dewaffled

2,850
2
17
30

what happens if you don't implement this following method? UniqueHandle& operator = (UniqueHandle&& other) – Daniel Fridman Jun 06 '20 at 11:55
@DanielFridman it is used to reassign value, e.g. `UniqueHandle a = ...; UniqueHandle b = ...; ... ; a = std::move(b);` - resource of `b` is reassigned to `a` and `b` value should not be used after move (it this example implementation the handle originally stored in `a` will be moved to `b` and released when `b` is destroyed but it is not required to be this way, the handle just should not leak). – dewaffled Jun 06 '20 at 12:05
@DanielFridman Then you can't assign the handle anymore. It'll heavily limit the usability of the object. Note that not implementing the method is not all: you have to actively prevent the object from being copied, usually by `= delete`-ing the copy constructor and assignment. – Kuba hasn't forgotten Monica Jun 06 '20 at 12:06
@ReinstateMonica I understand that, but why isn't the default imp of the move assignment enough? – Daniel Fridman Jun 06 '20 at 13:21
@dewaffled ok, but why didn't you use the default move assigment? – Daniel Fridman Jun 06 '20 at 13:24
Because who knows what it takes to move the "handle". The handle can be anything, not necessarily an `int` file descriptor :) It'd be probably ill-advised to let the compiler move such handles willy-nilly. It'd certainly come up on a code review if I was the reviewer. – Kuba hasn't forgotten Monica Jun 06 '20 at 13:24
@ReinstateMonica Ok thanks :) but why swiping is neccessery? why not just move the raw handle from the r-value? – Daniel Fridman Jun 06 '20 at 13:39
@DanielFridman swapping is not necessary, you just need to ensure the handle is not leaked or double-freed (destructor would still be called for rvalue), i.e. `if (handle_ != INVALID_HANDLE_VALUE) close_fn(handle_); handle_ = other.handle_; other.handle_ = INVALID_HANDLE_VALUE` is also ok, swap is just shorter to write. – dewaffled Jun 06 '20 at 14:04

score 0 · Answer 2 · answered Jun 06 '20 at 10:59

Ok, so I created an inner class called 'FindVolumeHandle to wrap the handle.

Could I done this in a better way?

DriverIterator.hpp:

class DriverIterator final
{
private:
    class FindVolumeHandle final
    {
    public:
        explicit FindVolumeHandle(const HANDLE handle);
        ~FindVolumeHandle() noexcept;

    private:
        HANDLE handle_;

    public:
        HANDLE get_handle() const;

    public:
        FindVolumeHandle(const FindVolumeHandle&) = delete;
        FindVolumeHandle(FindVolumeHandle&&) = delete;
        FindVolumeHandle& operator=(const FindVolumeHandle&) = delete;
        FindVolumeHandle& operator=(FindVolumeHandle&&) = delete;
    };

public:
    explicit DriverIterator();

private:
    std::unique_ptr<FindVolumeHandle> wrapped_handle_;
    std::unique_ptr<Driver> current_;
    bool is_empty_;

public:
    bool is_empty() const;
    Driver get_current() const;
    void next();

private:
    void initialize();

public:
    DriverIterator(const DriverIterator&) = delete;
    DriverIterator(DriverIterator&&) = delete;
    DriverIterator& operator=(const DriverIterator&) = delete;
    DriverIterator& operator=(DriverIterator&&) = delete;
};

DriverIterator.cpp:

DriverIterator::FindVolumeHandle::FindVolumeHandle(const HANDLE handle) :
    handle_(handle)
{}

DriverIterator::FindVolumeHandle::~FindVolumeHandle() noexcept
{
    try
    {
        FindVolumeClose(handle_);
    }
    catch (...)
    {

    }
}

HANDLE DriverIterator::FindVolumeHandle::get_handle() const
{
    return handle_;
}


DriverIterator::DriverIterator()
{
    initialize();
}

void DriverIterator::initialize()
{
    static constexpr uint32_t MAX_BUFFER_SIZE = 1024;
    wchar_t buffer[MAX_BUFFER_SIZE];

    HANDLE handle = FindFirstVolumeW(buffer, MAX_BUFFER_SIZE);
    if (handle == INVALID_HANDLE_VALUE)
    {
        //throw exception
    }

    wrapped_handle_ = std::make_unique<FindVolumeHandle>(handle);

    wchar_t paths[1024];
    DWORD   res_size;

    if (!GetVolumePathNamesForVolumeNameW(buffer, paths, 1024, &res_size))
    {
        //throw exception
    }

    current_ = std::make_unique<Driver>(paths);

    is_empty_ = false;
}

bool DriverIterator::is_empty() const
{
    return is_empty_;
}

Driver DriverIterator::get_current() const
{
    if (is_empty_)
    {
        //throw exception
    }

    return *current_;
}

void DriverIterator::next()
{
    //code..
}

You can merge the initialize function into the constructor - the extra indirection does not seem to pay a role — Michael Veksler, Jun 06 '20 at 11:14
You don't need to store your wrapper in `unique_ptr`, just define move constructor and move assignment operator in the wrapper itself. Also you can use a helper generic library like [microsoft/wil](https://github.com/microsoft/wil/wiki/RAII-resource-wrappers#wilunique_any) to declare such wrappers with minimal manual work. — dewaffled, Jun 06 '20 at 11:15
@dewaffled but then I need to initialize wrapped_handle_ in the initialization list of DriverIterator constructor, am i right? — Daniel Fridman, Jun 06 '20 at 11:18
@DanielFridman That's why you can use INVALID_HANDLE_VALUE with the default constructor, or use std::optional — Michael Veksler, Jun 06 '20 at 12:02

Throwing exception from constructor memory leak (handle wrapper c++)

2 Answers2