How to fix this typical exception unsafe code?

Question

According to GOTW #56, there is a potential classic memory leak and exception safety issues in the following code:

//  In some header file:
void f( T1*, T2* );
//  In some implementation file:
f( new T1, new T2 );

The reason is that when we new T1, or new T2, there might be exceptions thrown from the classes' constructors.

Meanwhile, according to the explanation:

Brief recap: An expression like "new T1" is called, simply enough, a new-expression. Recall what a new-expression really does (I'll ignore placement and array forms for simplicity, since they're not very relevant here):

• it allocates memory

• it constructs a new object in that memory

• if the construction fails because of an exception the allocated memory is freed

So each new-expression is essentially a series of two function calls: one call to operator new() (either the global one, or one provided by the type of the object being created), and then a call to the constructor.

For Example 1, consider what happens if the compiler decides to generate code as follows:

1: allocate memory for T1
2: construct T1
3: allocate memory for T2
4: construct T2
5: call f()

The problem is this: If either step 3 or step 4 fails because of an exception, the C++ standard does not require that the T1 object be destroyed and its memory deallocated. This is a classic memory leak, and clearly not a Good Thing. [...]

By reading more:

Why doesn't the standard just prevent the problem by requiring compilers to Do The Right Thing when it comes to cleanup?

The basic answer is that it wasn't noticed, and even now that it has been noticed it might not be desirable to fix it. The C++ standard allows the compiler some latitude with the order of evaluation of expressions because this allows the compiler to perform optimizations that might not otherwise be possible. To permit this, the expression evaluation rules are specified in a way that is not exception-safe, and so if you want to write exception-safe code you need to know about, and avoid, these cases. (See below for how best to do this.)

So my questions are:

1. How to fix this typical exception unsafe code? Should we simply avoid writing code like this?

2. The answer confused me a little bit, in order to handle constructor failures, we should throw exception from constructor according to C++ FAQ and make sure memory allocated are released properly, so assuming that the class T did implement code that handles construction failure, do we still have exception safety issue in the above code?

Thank you for your time and help.

Answer 1

First, write make_unique:

template<typename T, typename... Args>
std::unique_ptr<T> make_unique( Args&&... args ) {
  return {new T(std::forward<Args>(args)...)};
}

which was not included in the standard, basically as an oversight. std::make_shared is, and make_unique will probably show up in C++14 or 17.

Second, change your function signature to:

//  In some header file:
void f( std::unique_ptr<T1>, std::unique_ptr<T2> );

and call it like:

f( make_unique<T1>(), make_unique<T2>() );

and the result is exception-safe code.

If T1 has a non-trivial constructors you want to use, you can just pass the arguments to make_unique<T1> and it perfectly forwards them to the constructor of T1.

There are issues with the multiple ways of constructing a T1 via () or {}, but nothing is perfect.

Answer 2

How to fix this typical exception unsafe code?

Use smart pointers. If your function takes naked pointers and you don't have control over that, you could do it like this:

std::unique_ptr<T1> t1(new T1);
std::unique_ptr<T2> t2(new T2);
// assuming f takes ownership of the pointers:
f(t1.release(), t2.release());

assuming that the class T did implement code that handles construction failure, do we still have exception safety issue in the above code?

Here exception safety has little to do with whether the constructors can throw or not: new itself could throw because it couldn't allocate the memory, and we're back to square one. So when allocating an object with new, one should always assume the construction can fail even when the constructor explicitly states it cannot throw (except of course when using new(std::nothrow) but this is yet another matter).

Answer 3

First: yes, avoid thread safety issues.

If you can't rewrite f, consider this:

auto t1 = std::make_unique<T1>(); //C++14
std::unique_ptr<T2> t2{new T2}; //C++11
f( t1.get(), t2.get() );        //or release(), depending on ownership policies of f

If you can however, do this:

void f(std::unique_ptr<T1>, std::unique_ptr<T2>);

//call:
f(make_unique<T1>(), make_unique<T2>());