0

I have std::set of shared pointers to objects of some type ( ints here are just for example ). What I need is to insert shared ptr's constructed from raw pointers. But when I'm trying to erase some of set elements ( again I have just a raw pointer ), I should construct shared_ptr and pass it to erase method ( it seems to me to be true because shared_ptr's comparison operators compare their raw pointers inside ) .

Code snippet, leading to SIGABRT:

std::set<std::shared_ptr<int>> sett;
int *rp = new int(5);
sett.emplace( rp );
sett.erase( std::shared_ptr<int>( rp ) );
Uroboros
  • 189
  • 2
  • 12

2 Answers2

4

This is not OK:

sett.erase( shared_ptr<int>( rp ) );

Here, rp is a pointer, so you construct an anonymous temporary shared_ptr, then you delete the value and memory it points to, then your anonymous temporary deletes it again.

You must not construct two different shared_ptrs pointing to the same object. If you need something along these lines, you can consider enable_shared_from_this instead. Or better yet, erase from the container without constructing a shared_ptr at all, by implementing a comparison function for the std::set which allows comparing with a raw pointer. For more on that, see: What are transparent comparators?

John Zwinck
  • 239,568
  • 38
  • 324
  • 436
  • Thanks, but I'm confused about `You must not construct two different shared_ptrs pointing to the same object`. It's shared, not unique... – Uroboros Feb 08 '18 at 11:55
  • 2
    @Uroboros: It is shared if the two pointers know about each other. That is, if p1 points to some object, p2 may be constructed from p1, but must not be constructed from the address of the object. There is no way for them to coordinate simply by the address of the object (where would they store their communicated data?), so you must "connect" them by constructing all shared pointers from the first one if one already exists. Another way to think about it: you usually should not construct shared pointers from raw pointers--only from `new` expressions or via `make_shared`. – John Zwinck Feb 08 '18 at 11:57
  • 8
    Imagine an old man is dying. On Monday he asks his son to arrange his funeral. On Tuesday he asks his daughter to arrange his funeral. If the son and daughter do not communicate with each other, there will be two funerals, and one will fail with an empty coffin exception. – John Zwinck Feb 08 '18 at 12:01
4

Quoting from cppreference

Constructing a new shared_ptr using the raw underlying pointer owned by another shared_ptr leads to undefined behavior.

When you did this:

sett.emplace( rp );

Because of implicit type conversion a shared_ptr was created and given the ownership of the memory location pointed by rp. Lets call it sp1(1) where (1) denotes the ref-count

When you call this: sett.erase( std::shared_ptr<int>( rp ) );

Following sequence of events happen:

  • A new shared_ptr owns the memory pointed by rp. shared_ptr thinks that the ref-count of the memory is 1. sp2(1)

  • erase is called. This invokes the default comparator. This may lead to deletion of the first shared_ptr sp1.

  • The destructor is invoked for sp1 and the ref count would go to 0. Since ref-count is 0, the memory is freed.

  • When erase returns sp2(1) destructor is called (since that was a temporary created for the function call). The reference count goes to 0 for the memory managed by sp2

  • The destructor unaware that the fate of the underlying memory had already been sealed by the deletion of sp1 calls delete on the memory again. Double delete disaster happens at this point.

That's why, either keep everything as shared_ptr right from creation of memory, or write a custom-comparator for your pointer types.

bashrc
  • 4,725
  • 1
  • 22
  • 49