Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Resource leak in Stroustrup example with std::uninitialized_copy?

Tags:

c++

In the Stroustrup book (The C++ programming language 4th ed., §17.5.1, pag 508) I found the following example of a copy constructor for a simple Matrix class:

template < class T >
Matrix:: Matrix( const Matrix& m ) // copy constructor
    : dim{ m.dim },
      elem{ new T[ m.size() ] }
{
    uninitialized_copy( m.elem, m.elem+m.size(), elem ); // copy elements
}

(where elem is the pointer to the array of T elements, declared as T* elem;).

I have two questions about this copy constructor:

  1. why the method first default construct an array of m.size() elements, only to overwrite it in the body with the uninitialized_copy call?

  2. with the initialization elem{ new T[ m.size() ] }, the constructor of T is called m.size() times. However, the algorithm uninitialized_copy in the body doesn't call the T destructor before constructing the new array in the same area. Is this a potential resource leak? (Note: not a memory leak, a resource leak! e.g., if T acquires a lock or a file descriptor in the ctor and releases it in the dtor).

Thanks

like image 631
Daniele Pallastrelli Avatar asked Jul 21 '14 07:07

Daniele Pallastrelli


2 Answers

I'm the author of the question.
I asked my two questions to Bjarne Stroustrup via email and thought to share his answers here.

He kindly (and shortly :-) answered:

  1. It's an oversight and a waste.
  2. Yes, if a type T holds resources, it's a leak

He also wrote he's going to fix the error.

like image 101
Daniele Pallastrelli Avatar answered Nov 09 '22 14:11

Daniele Pallastrelli


Yes, this is clearly an error; the code should use copy instead of uninitialized_copy. Since T is usually going to be a trivial type, the resource leak is usually going to be avoided, but it's still a bug.

To answer your first question, it's simpler to write the copy constructor member initializer default constructing an array, as that allows the copy constructor to use the same simple allocation method as the (n, m) constructor. It would be more efficient to allocate an uninitialized memory area and construct into it, but that would complicate the constructors and destructors, which would detract from the point of the Matrix class. The more idiomatic way to write this would be with Matrix wrapping a dynamic array (fixed-size vector) class, for example dynarray (possibly post-C++14 TS).

As Puppy has pointed out, the copy constructor has a memory leak if its body throws. This would happen regardless of whether copy or uninitialized_copy is used. Stroustrup does say // simplified (no error handling) earlier in the class, so this can be excused.

At a guess, I'd say the way this error crept in was that Stroustrup uses uninitialized_copy elsewhere in the book, particularly in his vector class. I don't have access to the older editions but I have a suspicion that this was added to replace a loop and try/catch block in the vector copy constructor, and Matrix was modified similarly without full consideration.

It's not shown at Stroustrup's list of errata for the 4th edition, but he may already be aware (the errata page was last updated November 2013). If you feel like emailing him with this error you should definitely do so!

like image 21
ecatmur Avatar answered Nov 09 '22 14:11

ecatmur