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...
}

Answer 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.

Answer 2

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..
}