Efficiently and elegantly returning emplaced unique_ptr

Question

I found out (thanks to a StackOverflow comment) a security flaw in my code:

std::vector<std::unique_ptr<Item>> items;

template<class... TS> Item& create(TS&&... mArgs)
{
    auto item(new Item(std::forward<TS>(mArgs)...);
    items.emplace_back(item); // Possible exception and memory leak
    return *item;
}

Basically, allocating the Item with a raw new may leak memory if the emplace_back throws.

The solution is never using a raw new, but using a std::unique_ptr right in the method body.

std::vector<std::unique_ptr<Item>> items;

template<class... TS> Item& create(TS&&... mArgs)
{
    auto item(std::make_unique<Item>(std::forward<TS>(mArgs)...);
    items.emplace_back(std::move(item));
    return *item; // `item` was moved, this is invalid!
}

As you can see, returning item is invalid, as I had to move item using std::move to emplace it into the items container.

I can't think of a solution that requires storing item's address in an additional variable. The original (buggy) solution is however very concise and easy to read.

Is there a more elegant way of returning an std::unique_ptr that was moved for emplacement in a container?

Why do you return a reference to something inside `unique_ptr` in the first place? — Joker_vD, Feb 20 '14 at 17:33
As an aside, note that the code only offers basic exception guarantees: if `emplace_back` throws, the parameters will have already been moved from. — avakar, Feb 20 '14 at 17:38
@avakar @VittorioRomeo I don't think it's a problem, the temporaries passed into it were going to be destroyed anyway. As for copies, it would be only a problem if copy constructor was non-`const` for some of the parameters. — Joker_vD, Feb 20 '14 at 17:41
@VittorioRomeo In cases of extreme paranoia, I suppose you could reserve space in `items` before touching the parameters - then you'd only lose information if the `Item` constructor throws. — Casey, Feb 20 '14 at 17:42

Jarod42 · Accepted Answer · 2014-02-20T17:35:48.247

8

You may write:

template<class... TS>
Item& create(TS&&... mArgs)
{
    items.emplace_back(std::make_unique<Item>(std::forward<TS>(mArgs)...));
    return *items.back();
}

edited Feb 20 '14 at 17:35

answered Feb 20 '14 at 17:29

Jarod42

203,559
14
181
302

Casey · Answer 2 · 2014-02-20T17:47:05.663

2

Caching the reference before the emplace is a more generally applicable option (e.g., for non-vector containers):

template<class... TS> Item& create(TS&&... mArgs)
{
    auto item = std::make_unique<Item>(std::forward<TS>(mArgs)...);
    auto& foo = *item;
    items.emplace_back(std::move(item));
    return foo; // This *is* valid.
}

edited Feb 20 '14 at 17:47

answered Feb 20 '14 at 17:36

Casey

41,449
7
95
125

thought i think you can still have problems if emplace throws, andrej had recently an article where he suggests calling .reserve() first – NoSenseEtAl Feb 21 '14 at 12:18
@NoSenseEtAl That's basically the issue we're discussing in the comments on the OP - although no one really made it clear. This is enough to provide the basic exception safety guarantee: if `emplace_back` throws, arguments passed to `create` as rvalues will be left in the moved-from condition with their guts torn out and destroyed. If it's desired to provide the strong exception safety guarantee, then space in the vector should be reserved `items.reserve(items.size() + 1)` *before* anything else happens, especially the potential movement of arguments into the `unique_ptr`. – Casey Feb 21 '14 at 17:10
1

Correction: this function *can't* provide the strong exception safety guarantee because there's no way to ensure that the `Item` constructor either succeeds or does not modify the `mArgs`. You can guarantee that (a) the container is unchanged if an exception is thrown, and (b) the arguments are unmodified unless the `Item` constructor throws. If the strong guarantee is critical, the function would have to be modified to accept a `unique_ptr` param and simply move it into the vector. – Casey Feb 21 '14 at 17:18

zmb · Answer 3 · 2014-02-20T18:36:48.127

2

Your question is tagged C++11, and other answers suggesting make_unique fail to mention that it is a C++14 feature. I believe this C++11 approach also resolves the leak.

#include <vector>
#include <memory>
#include <utility>
#include <iostream>

struct Item
{
   int a, b;
   Item(int aa, int bb) : a{aa}, b{bb} { }
};

static std::vector<std::unique_ptr<Item>> items;

template <class... Ts> Item& create(Ts&&... args)
{
    items.emplace_back(std::unique_ptr<Item>{new Item(std::forward<Ts>(args)...)});
    return *items.back();
}

int main()
{
    Item& x = create(1, 2);
    std::cout << "( " << x.a << ", " << x.b << " )" << std::endl;
}

This should be safe because emplace_back() cannot be called until a unique_ptr<Item> was already constructed, so even if emplace_back() does throw, your Item is already managed by the unique_ptr.

edited Feb 20 '14 at 18:36

answered Feb 20 '14 at 18:04

zmb

7,605
4
40
55

Where is this `unique_ptr` constructed in `create`? I'm not seeing it. Did you mean to write `items.emplace_back(std::unique_ptr{new Item(std::forward(args)...)});`? – Casey Feb 20 '14 at 18:13
I guess I was thinking that a `unique_ptr` would have to be implicitly constructed from the arguments to emplace_back, but I see now that that's not right. Yes, your suggestion looks better. – zmb Feb 20 '14 at 18:31

Efficiently and elegantly returning emplaced unique_ptr

3 Answers3