Top 10 dumb mistakes to avoid with C++ 11 smart pointers

I love the new C++ 11 smart pointers. In many ways, they were a godsent for many folks who hate managing their own memory. In my opinion, it made teaching C++ to newcomers much easier.

However, in the two plus years that I've been using them extensively, I've come across multiple cases where improper use of the C++ 11 smart pointers made the program inefficient or simply crash and burn. I've catalogued them below for easy reference. 

Before we begin, let's take a look at a simple Aircraft class we'll use to illustrate the mistakes.

class Aircraft
{
private:
	string m_model;

public:

	int m_flyCount;

	weak_ptr myWingMan;

	void Fly()
	{
		cout << "Aircraft type" << m_model << "is flying !" << endl;
	}

	Aircraft(string model)
	{
		m_model = model;
		cout << "Aircraft type " << model << " is created" << endl;
	}

	Aircraft()
	{
		m_model = "Generic Model";
		cout << "Generic Model Aircraft created." << endl;
	}

	~Aircraft()
	{
		cout << "Aircraft type  " << m_model << " is destroyed" << endl;
	}

};

Mistake # 1 : Using a shared pointer where an unique pointer suffices !!!

I’ve recently been working in an inherited codebase which uses a shared_ptr for creating and managing every object. When I analyzed the code, I found that in 90% of the cases, the resource wrapped by the shared_ptr is not shared.

This is problematic because of two reasons:

1. If you have a resource that’s really meant to be owned exclusively, using a shared_ptr  instead of a unique_ptr makes the code susceptible to unwanted resource leaks and bugs.

  • Subtle Bugs: Just imagine if you never imagined a scenario where the resource is shared out by some other programmer by  assigning it to another shared pointer which inadvertently modifies the resource !
  • Unnecessary Resource Utilization: Even if the other pointer does not modify the shared resource, it might hang on to it far longer than necessary thereby hogging your RAM unnecessarily even after the original shared_ptr goes out of scope.

2. Creating a shared_ptr is more resource intensive than creating a unique_ptr.

  • A shared_ptr needs to maintain the threadsafe refcount of objects it points to and a control block under the covers which makes it more heavyweight than an unique_ptr.

Recommendation – By default, you should use a unique_ptr. If a requirement comes up later to share the resource ownership, you can always change it to a shared_ptr.

Mistake # 2 : Not making resources/objects shared by shared_ptr threadsafe !

Shared_ptr allows you to share the resource thorough multiple pointers which can essentially be used from multiple threads. It’s a common mistake to assume that wrapping an object up in a shared_ptr makes it inherently thread safe. It’s still your responsibility to put synchronization primitives around the shared resource managed by a shared_ptr.

Recommendation – If you do not plan on sharing the resource between multiple threads, use a unique_ptr.

Mistake # 3 : Using auto_ptr !

The auto_ptr feature was outright dangerous and has now been deprecated. The transfer of ownership executed by the copy constructor when the pointer is passed by value can cause fatal crashes in the system when the original auto pointer gets dereferenced again. Consider an example:

int main()
{
	auto_ptr myAutoPtr(new Aircraft("F-15"));
	SetFlightCountWithAutoPtr(myAutoPtr); // Invokes the copy constructor for the auto_ptr
	myAutoPtr->m_flyCount = 10; // CRASH !!!
}

Recommendation – unique_ptr does what auto_ptr was intended to do. You should do a search and find on your codebase and replace all auto_ptr with unique_ptr. This is pretty safe but don’t forget to retest your code ! πŸ™‚

Mistake # 4 : Not using make_shared to initialize a shared_ptr !

make_shared has two distinct advantages over using a raw pointer:

1. Performance : When you create an object with new , and then create a shared_ptr , there are two dynamic memory allocations that happen : one for the object itself from the new, and then a second for the manager object created by the shared_ptr constructor.

shared_ptr pAircraft(new Aircraft("F-16")); // Two Dynamic Memory allocations - SLOW !!!

On the contrary, when you use make_shared, C++ compiler does a single memory allocation big enough to hold both the manager object and the new object.

shared_ptr pAircraft = make_shared("F-16"); // Single allocation - FAST !

2. Safety: Consider the situation where the Aircraft object is created and then for some reason the shared pointer fails to be created. In this case, the Aircraft object will not be deleted and will cause memory leak ! After looking at the implementation in MS compiler memory header I found that if the allocation fails, the resource/object is deleted. So Safety is no longer a concern for this type of usage.

Recommendation: Use make_shared to instantiate shared pointers instead of using the raw pointer.

Mistake # 5 : Not assigning an object(raw pointer) to a shared_ptr as soon as it is created !

An object should be assigned to a shared_ptr as soon as it is created. The raw pointer should never be used again.

Consider the following example:

int main()
{
	Aircraft* myAircraft = new Aircraft("F-16");

	shared_ptr pAircraft(myAircraft);
	cout << pAircraft.use_count() << endl; // ref-count is 1

	shared_ptr pAircraft2(myAircraft);
	cout << pAircraft2.use_count() << endl; // ref-count is 1

	return 0;
}

     It’ll cause an ACCESS VIOLATION and crash the program !!!

 

    The problem is that when the first shared_ptr goes out of scope, the myAircraft object is destroyed. When the second shared_ptr goes out of scope , it tries to destroy the previously destroyed object again !

 

Recommendation: If you’re not using make_shared to create the shared_ptr , at least create the object managed by the smart pointer in the same line of code – like :

shared_ptr pAircraft(new Aircraft("F-16"));

Mistake # 6 : Deleting the raw pointer used by the shared_ptr !

You can get a handle to the raw pointer from a shared_ptr using the shared_ptr.get() api. However, this is risky and should be avoided. Consider the following piece of code:

void StartJob()
{
	shared_ptr pAircraft(new Aircraft("F-16"));
	Aircraft* myAircraft = pAircraft.get(); // returns the raw pointer
	delete myAircraft;  // myAircraft is gone
}

Once we get the raw pointer (myAircraft) from the shared pointer, we delete it. However, once the function ends, the shared_ptr pAircraft goes out of scope and tries to delete the myAircraft object which has already been deleted. The result is an all too familiar ACCESS VIOLATION !

 

Recommendation: Think really hard before you pull out the raw pointer from the shared pointer and hang on to it. You never know when someone is going to call delete on the raw pointer and cause your shared_ptr to Access Violate.

 

Mistake # 7 : Not using a custom deleter when using an array of pointers with a shared_ptr !

Consider the following piece of code:

void StartJob()
{
	shared_ptr ppAircraft(new Aircraft[3]);
}

The shared pointer will just point to Aircraft[0] — Aircraft[1] and Aircraft[2] have memory leaks will not be cleaned up when the smart pointer goes out of scope. If you’re using Visual Studio 2015, you’ll get a heap corruption error.

 

Recommendation: Always pass a custom delete with array objects managed by shared_ptr. The following code fixes the issue:

void StartJob()
{
	shared_ptr ppAircraft(new Aircraft[3], [](Aircraft* p) {delete[] p; });
}

Mistake # 8 : Not avoiding cyclic references when using shared pointers !

In many situations , when a class contains a shared_ptr reference , you can get into cyclical references. Consider the following scenario – we want to create two Aircraft objects – one flown my Maverick and one flown by Iceman ( I could not help myself from using the TopGun reference !!! ). Both maverick and Iceman needs to hold a reference to each Other Wingman.

 

So our initial design introduced a self referencial shared_ptr inside the Aircraft class:

class Aircraft
{
private:
       string m_model;
public:
       int m_flyCount;
       shared_ptr<Aircraft> myWingMan;
….

Then in our  main() , we create Aircraft objects, Maverick and Goose , and make them each other’s wingman:

 

 

int main()
{
	shared_ptr pMaverick = make_shared("Maverick: F-14");
	shared_ptr pIceman = make_shared("Iceman: F-14");

	pMaverick->myWingMan = pIceman; // So far so good - no cycles yet
	pIceman->myWingMan = pMaverick; // now we got a cycle - neither maverick nor goose will ever be destroyed

	return 0;
}

When main() returns, we expect the two shared pointers to be destroyed – but neither is because they contain cyclical references to one another. Even though the smart pointers themselves gets cleaned from the stack, the objects holding each other references keeps both the objects alive.

Here's the output of running the program:

Aircraft type Maverick: F-14 is created

Aircraft type Iceman: F-14 is created

 

So what’s the fix ? we can change the shared_ptr inside the Aircraft class to a weak_ptr ! Here’s the output after re-executing the main().

 

Aircraft type Maverick: F-14 is created

Aircraft type Iceman: F-14 is created

Aircraft type  Iceman: F-14 is destroyed

Aircraft type  Maverick: F-14 is destroyed

 

Notice how both the Aircraft objects were destroyed.

 

Recommendation: Consider using weak_ptr in your class design when ownership of the resource is not needed and you don’t want to dictate the lifetime of the object.

 

Mistake # 9 : Not deleting a raw pointer returned by unique_ptr.release() !

The Release() method does not destroy the object managed by the unique_ptr, but the unique_ptr object is released from the responsibility of deleting the object. Someone else (YOU!) must delete this object manually.

 

The following code below causes a memory leak because the Aircraft object is still alive at large once the main() exits.

int main()
{
	unique_ptr myAircraft = make_unique("F-22");
	Aircraft* rawPtr = myAircraft.release();
	return 0;
}

Recommendation: Anytime you call Release() on an unique_ptr, remember to delete the raw pointer. If your intent is to delete the object managed by the unique_ptr, consider using unique_ptr.reset().

Mistake # 10 : Not using a expiry check when calling weak_ptr.lock() !

Before you can use a weak_ptr, you need to acquire the weak_ptr by calling a lock() method on the weak_ptr. The lock() method essentially upgrades the weak_ptr to a shared_ptr such that you can use it. However, if the shared_ptr object that the weak_ptr points to is no longer valid, the weak_ptr is emptied. Calling any method on an expired weak_ptr will cause an ACESS VIOLATION.

 

For example, in the code snippet below, the shared_ptr that “mywingMan” weak_ptr is pointing to has been destroyed via pIceman.reset(). If we execute any action now via myWingman weak_ptr, it’ll cause an access violation.

int main()
{
	shared_ptr pMaverick = make_shared("F-22");
	shared_ptr pIceman = make_shared("F-14");

	pMaverick->myWingMan = pIceman;
	pIceman->m_flyCount = 17;

	pIceman.reset(); // destroy the object managed by pIceman

	cout << pMaverick->myWingMan.lock()->m_flyCount << endl; // ACCESS VIOLATION

	return 0;
}

It can be fixed easily by incorporating the following if check before using the myWingMan weak_ptr.

	if (!pMaverick->myWingMan.expired())
	{
		cout << pMaverick->myWingMan.lock()->m_flyCount << endl;
	}

EDIT: As many of my readers pointed out, the above code should not be used in a multithreaded environment – which equates to 99% of the software written nowadays. The weak_ptr might expire between the time it is checked for expiration and when the lock is acquired on it. A HUGE THANKS to my readers who called it out ! I'll adopt Manuel Freiholz's solution here : Check if the shared_ptr is not empty after calling lock() and before using it.

shared_ptr<aircraft> wingMan = pMaverick->myWingMan.lock();
if (wingMan)
{
	cout << wingMan->m_flyCount << endl;
}

Recommendation: Always check if a weak_ptr is valid – actually if a non-empty shared pointer is returned via lock() function before using it in your code.

So, What's Next ?

If you want to learn more about the nuances of C++ 11 smart pointers or C++ 11 in general, I recommend the following books.

1. C++ Primer (5th Edition) by Stanley Lippman

2. Effective Modern C++: 42 Specific Ways to Improve Your Use of C++11 and C++14 by Scott Meyers

All the best in your journey of exploring C++ 11 further. Please share if you liked the article. πŸ™‚

 

  • Manuel Freiholz

    Hi. In addition to mistake #10, it might be worth to mention that this is not thread-safe. A thread-safe approach would look like this:


    shared_ptr wingMan = pMaverick->myWingMan.lock();
    if (wingMan)
    {
    cout <m_flyCount << endl;
    }

    • Great point Manuel – thanks for sharing. I’ve updated the post to reflect the reality of multithreaded environments with your solution.

  • hcg2007

    The fix for #10 is wrong. You should not call expired(). You must test the valued returned by lock() as Manuel Freiholz suggests.

    • Yup – thanks for calling it out – I’ve edited the post with Manuel’s solution.

  • Hacker-T

    These aren’t dumb at all. Title should have been Top ten human mistakes to avoid with C++11

    • My apologies if the title offended you. My intention was not to call people making these mistakes dumb – but rather juxtaposing certain “dumbness” in “smart” pointers. Sorry if it came out in the wrong way.

      I’ve made a bunch of these mistakes myself – these just happen because of nuances of the language and C++ leaves a lot of room to shoot yourself in the foot.

  • Francisco Miguel GarcΓ­a

    In a multithread app, #10 could generate a race-condition. A proper way is to store the shared_ptr returned by “lock()”, check if it is null and operate it.

    • Agreed – thanks Francisco. The post has been edited πŸ™‚

  • Marcelo Cantos

    The given solution to #10 is flat-out wrong (not thread-safe, as another poster points out). The best solution is to lock and test in one go:


    if (auto wingMan = pMaverick->myWingMan.lock()) {
    cout <m_flyCount << endl;
    }

    This has the advantage of making it impossible to accidentally refer to wingMan when it’s null.

    • Yup – should be fixed now.

      • Marcelo Cantos

        Sure, but the advice to not use the original version in multithreaded code is still wrong. The original version should never be used at all. It has no advantages, and the disadvantage β€” even in the single-threaded case β€” that it has to check twice for pointer validity. It’s unlikely the compiler can optimise away the double-check, since the compiler cannot assume a single-threaded environment.

        • Marcelo, so I looked at the implementation of each in VC++ compiler. If the object has indeed expired, in a single threaded case this is cheaper because you don’t invoke the shared pointer constructor and hinge on a simple count kept internally. That’s the theoretical side. Practically, when I profiled both solutions in a loop 1M times, I found no difference in performance. If you find evidence to the contrary, I’d be curious to know and happy to edit the article.

          • Marcelo Cantos

            Even if you could show that performance is never impacted with any compiler on any OS, it’s still the wrong advice. Performance is just one of several problems with your original solution.

  • Anders

    Regarding Mistake #7, I don’t think you should be advocating or suggesting raw arrays. I think the recommendation should be that if you think about using an array, just use a vector. And then you can use your implicit recommendation #0 to make each element a smart pointer and use a std::vector<std::unique_ptr>.

    • Nope, I’m not recommending using raw arrays. This was more of an illustrative point in case someone already has this in their codebase. Like you mentioned, using Vectors is preferable in majority of circumstances. It’s interesting you should mention vector of unique_ptr – that’s what I typically tend to use for most of my needs.

  • Johannes Schaub

    “Mistake # 10 : Not using a expiry check when calling weak_ptr.lock()”. Some designs don’t require you to do this ceck: Suppose you have a Node class that itself creates a child. The child is passed the parent’s weak_ptr. Any code wants the child has to call parent->getChild(). Herein lies the trick. getChild() can be implemented as `return std::shared_ptr(shared_from_this(), m_child.data())`. Therefore, the parent keeps existing as long as someone else has a child node pointer. But there still is no dependency cycle.

    • Good point Johannes. This is an interesting way to design self referencial data structures. Thanks for sharing.

  • Nir

    My comment from reddit, reproduced here:

    One minor thing to be aware of with make_shared (#4) is that if you are planning to use it with weak pointers, there is a disadvantage as well as an advantage. Namely, you will not recollect the memory from the object as long as there are weak pointers, even if the last shared_pointer has gone out of scope and the object has been destroyed. Because the object and control block are allocated together, they also must be deallocated together, and the control block needs to be around as long as weak pointers are. If you do not use make_shared, then they are allocated separately so this issue does not exist.

    • Wow – interesting point – I didn’t know that. Thanks Nir. So I wonder, if the last shared pointer has gone out of scope, the weak pointers pointing to this object is already expired – it’s weird that the control block is not de-allocated at that point.

  • R Schubert

    #4.2 is wrong: If you pass a naked pointer to a shared_ptr constructor, and the constructor call fails, delete is called on the passed-in pointer. Therefore, even if in the shared_ptr constructor fails, no memory is leaked:

    shared_ptr pAircraft(new Aircraft("F-16")); // Exception safe!

    Therefore, and in spirit with #5, you should never attempt anything like in this stupid example:


    shared_ptr pMaverick;
    aircraft *plane = new Aircraft("F-22");
    try {
    pMaverick = shared_ptr(plane);
    } catch {

    }

    Also, #7 got fixed for C++17.

  • R Schubert

    #4.2 is wrong: If you try to construct a shared_ptr from a naked pointer and the shared_ptr constructor fails, delete is called on the passed-in pointer. Therefore, this is perfectly safe:

    shared_ptr<Aircraft> pAircraft(new Aircraft("F-16")); // Exception safe

    Also, #7 will be fixed in C++17.

    • Thanks for sharing this. I got a little confused by the sentence structure in the standard :

      Throws: bad_alloc, or an implementation-defined exception when a resource other than memory
      could not be obtained.

      I checked the implementation in Microsoft VS2015 compiler and they seem to delete the resource if any exception (catch(…) ) happens.

      template
      explicit shared_ptr(_Ux *_Px)
      { // construct shared_ptr object that owns _Px
      _Resetp(_Px);
      }

      template
      void _Resetp(_Ux *_Px)
      { // release, take ownership of _Px
      _TRY_BEGIN // allocate control block and reset
      _Resetp0(_Px, new _Ref_count(_Px));
      _CATCH_ALL // allocation failed, delete resource

      delete _Px;
      _RERAISE;
      _CATCH_END
      }

      Do you know if the same is true for other compilers ? I’ll check it before editing the post.

      • R Schubert

        Item no. 7 of N3242 Β§20.7.2.2.1 (shared_ptr constructors) states: Exception safety: If an exception is thrown, delete p is called.

        • Cool. I edited the post to reflect this information.

  • Eric

    Informative article. I feel compelled to point out a few things with the Aircraft class example – Namely, the constructors.
    – The initializer list is not used (instead, m_model is assigned in the constructor body)
    – m_flyCount is not initialized
    – The ‘model’ parameter should be passed as a const reference for efficiency

    For example:

    Aircraft(const string& model)
    : m_flyCount(0), m_model(model)
    {
    cout << "Aircraft type " << model << " is created" << endl;
    }

    Aircraft()
    : m_flyCount(0), m_model("Generic Model")
    {
    cout << "Generic Model Aircraft created." << endl;
    }

    • Yup – good points – all those should be taken care of when writing production level code.

  • Darrell Wright

    With #10, you now are open to the possibility of using wingMan when it isn’t valid outside the if scope. Better to put the declaration/definition inside the if statement like Marcelo did e.g.

    if( auto wingMan = pMaverick->myWingman.lock( ) ) { …. } or doing the opposite and something like

    if( !wingMan ) { return… }

  • Really nice blog. I learned a lot from it. Thanks!

    • Thanks Ryan. I’m really glad you found the content helpful. πŸ™‚

  • Pingback: C++ Annotated: March – May 2016 | CLion Blog()

  • Pingback: C++ Annotated: March – May 2016 | ReSharper C++ Blog()

  • Galik

    I don’t really agree with everything in #6.

    In the new pointer world I think the advice should be to never, ever, ever delete a raw pointer. But they should definitely be used and passed in preference to passing the shared pointer itself – unless you also need to pass ownership responsibilities. Otherwise you force all your functions to accept shared-pointer parameters, which is inefficient and restrictive. It is restrictive because it means that only one type of smart pointer can be passed to a function but the function should not have to care what type of smart pointer is managing the resource it wishes to process. It should receive a reference to the object it cares about (or a pointer).

    So I think raw pointers (and references to their objects) have an important role to play in a universe where all resources are owned using smart pointers (or containers). The role is that raw pointers are never ever to be deleted because they are always assumed to be owned by someone else. This is enforced by your previous rules that say you never create an object using a raw pointer and, if you do, you immediately assign it to a smart pointer to manage its lifespan.

    • Thanks Galik. That’s actually pretty valuable advice – never thought it that way – but yeah, I suppose you’ll need to make all your functions use the same type of pointer. Not quite sure about the perf aspect – did you notice any difference in the profiler if a smart pointer is passed to a function vs a naked pointer ? I’ll need to run this experiment next time I’m profiling my project.

  • Galik

    I find #9 a bit strange. I think it is definitely a big source of mistakes but it seems to me the mistake is in calling release() instead of calling reset(). It doesn’t really make sens to call release() only to then delete the object. That’s dangerous. If it is to be deleted then you should call reset().

    I would think the major reason for calling release() would be to transfer ownership to another smart pointer (a shared pointer for example).

    • That makes sense. I’ve actually never had top use release() myself in production code thus far. The preferred patterns we use is using Reset for deleting the pointer or Reset it with a new underlying object.
      The code was for illustrative purposes only πŸ™‚

  • Galik

    Mistake #8 has an alternate solution which I find compelling when dealing with complex relationships like networks. It is both safe and performant if you store your actual objects in a container (preferably a vector) and implement the relationships between them using raw pointers. The container manages the objects’ lifespans so there are no leaks and the code to build and process the connections is fast and easy. As always you hang by the rule of never ever ever deleting a raw pointer. Once the resource they refer to is managed by a container or smart pointer you can treat them like integers or any other fundamental type.

    • So use something like std::vector httpClientVec ? How does the HttpClient* objects get deleted ? Just trying to understand – do you have a code sample ?

  • Christian Abel

    “we can change the shared_ptr inside the Aircraft class to a weak_ptr !”

    What is the dependency relation between these objects?

    • it’s self referential – was that the question Christian ? Sorry if i misunderstood – can you please clarify the question ?

      • Christian Abel

        What is the exact dependency relationship?

        • Each Aircraft object has a pointer to another Aircraft object whose lifetime it does not control.

          • Christian Abel

            If the mere existence of an object A doesn’t control the lifetime of B, then A probably should never contain a “reference” or owning pointer to B.

            That is true with or without cycles.

  • Nick DeWaal

    Can we not use so many exclamation marks? It’s extremely distracting from the message making it hard to read. Thank you for the really useful information though. πŸ™‚ https://youtu.be/VSKn8RlD7Is

    • Haha ! Yes – thank you for the feedback – i was clearly too excited when i wrote this piece coming fresh out of a code review.

  • Chuy Max

    This is wrong: “Calling any method on an expired weak_ptr will cause an ACESS VIOLATION.”. I haven’t checked the standard, but I think the correct answer is undefined behavior. Try calling a non-virtual method that does not use object attributes and check for yourself if the application crashes with an access violation error.

    • Have you tried it in MSVC++ compiler ?

  • Saurabh Bhairava

    Do you mean Goose or Iceman?

  • Krzysiek

    Wow. Great tips! It looks that I was failing in most of it. I’ve learned a lot from this. Thank you Sir!

    • Thank you ! So glad you found it helpful πŸ™‚

      • MrGerdbrecht

        Good written and informative stuff, thank you.

  • Reza Nezami

    #2 recommendation is wrong. There are valid uses of shared_ptr in single-threaded environment. Its reason d’etre has in fact nothing to do with multi-threadedness. Shared_ptr is for when there is not explicit owner of a ptr and idea is to avoid hanging ptr when no one seems to use it anymore.