How to avoid dangling pointer with shared_ptr?

Question

I have a class whose object pointers will be added as a key/data in multiple std::map/std::unordered_map/hash(internally implemented). In order to automate deletion of the object I am using shared_ptr.

I have designed my class using shared_ptr only class.

Now I want to make sure that in future no one does this:

#include <memory>
#include <string>

class A {
 protected:
   struct this_is_private;

 public:
   explicit A(const this_is_private &) {}
   A(const this_is_private &, ::std::string, int) {}

   template <typename... T>
   static ::std::shared_ptr<A> create(T &&...args) {
      return ::std::make_shared<A>(this_is_private{0},
                                   ::std::forward<T>(args)...);
   }

 protected:
   struct this_is_private {
       explicit this_is_private(int) {}
   };

   A(const A &) = delete;
   const A &operator =(const A &) = delete;
};


::std::map<A*, int> m_error;
::std::map<::std::shared_ptr<A>, int> m_ok;

::std::shared_ptr<A> foo()
{
   ::std::shared_ptr<A> temp = A::create();

   A * obj_ptr = temp.get(); 
   m_error.insert(pair<A*, int>(obj_ptr, 10)); //How to make sure no one do this in future
   m_ok.insert(pair<::std::shared_ptr<A>, int>(temp,10)); //ok
}

People will always be able to make raw pointers to existing objects. You have already made it impossible to create an `A` without using your `create` function. If someone really wants to shoot themselves in the foot, you can't stop them. — super, Jul 09 '19 at 14:33
Like super said, you can't stop people from shooting themselves in the foot. One thing you can do though is have a good code review process and require the usage of `make_unique` and `make_shared` when creating pointers. — NathanOliver, Jul 09 '19 at 14:38
If you want to prevent people from misusing global m_error, then put m_error inside a class and control how it is manipulated by using the class's public API. The public API of std::map of a global object gives everyone free rein, which works if everyone is diligent and disciplined. — Eljay, Jul 09 '19 at 14:41
You could write a wrapper class that wraps a `shared_ptr` stored internally. But ultimately people could take/store pointers of _that_ too. Not to mention that you could just as easily store references to `A` (by just dereferencing a `shared_ptr `) which is even more impossible to avoid. — Max Langhof, Jul 09 '19 at 14:41
For clarification of your question: More generally speaking, do you want to prevent people getting raw pointers to your class A's objects? — Lorenz Zhao, Jul 09 '19 at 14:48
@LorenzZhao I got confused with my last comment. Yes you are right that is what I want. — nav_jan, Jul 09 '19 at 14:53
@MaxLanghof yes I understand it now after reading the comments. — nav_jan, Jul 09 '19 at 14:55
You can `delete` the `operator&` for your class to prevent anyone from taking its addresses the simple way, but there's also [`std::addressof`](https://en.cppreference.com/w/cpp/memory/addressof) and, of course, `std::shared_ptr::get` (and like 10 more ways) that you can't possibly prevent. https://godbolt.org/z/vDw801 — Max Langhof, Jul 09 '19 at 14:56

eerorika · Accepted Answer · 2019-07-09T15:07:28.580

How to avoid dangling pointer with shared_ptr?

By not storing bare pointers (nor references nor iterators) to the object at all, or ensuring that the lifetime of such pointer is shorter than the shared pointer's lifetime. Correctness of latter is not as easy to prove as the correctness of the former.

How to make sure no one [store bare pointers] in future

There is no feature in C++ language that would prevent taking the address of an object and storing it other than encapsulating access to the object completely. It is always the responsibility of the programmer who takes the address to make sure that the lifetime is - and will be in future - what they expect. And it is the responsibility of the programmer who changes the lifetime of an object to ensure that nothing depends on the changed lifetime.

There are programming languages, that have been designed so as to not let the programmer access the address of an object directly, thereby making this class of bug impossible. C++ is not one of those languages.

score 1 · Answer 2 · answered Jul 09 '19 at 16:00

Hide your shared_ptr in a wrapper class "HiddenSharedPointer", so the user of your class gets no direct access to the object.

#include <memory>
#include <list>
#include <utility>

class A 
{
public:
    class HiddenSharedPointer : private std::shared_ptr<A>
    {
    public:
        template<typename... Args>
        explicit HiddenSharedPointer(Args&&... args);
        HiddenSharedPointer(HiddenSharedPointer&) = default;
        HiddenSharedPointer(const HiddenSharedPointer&) = default;
        HiddenSharedPointer(HiddenSharedPointer&&) = default;
        HiddenSharedPointer& operator=(const HiddenSharedPointer&) = default;
        HiddenSharedPointer& operator=(HiddenSharedPointer&&) = default;

        // methods directly called on the underlying shared_ptr
        using std::shared_ptr<A>::reset;

        // methods called on the object referenced by the underlying shared_ptr
        void foo();
    };
private:
    explicit A()
    {}
    A(::std::string, int)
    {}
    A(const A &) = delete;
    const A &operator =(const A &) = delete;
public:
    template <typename... T>
    static HiddenSharedPointer create(T &&...args)
    {
        return HiddenSharedPointer(::std::forward<T>(args)...);
    }

    void foo()
    {
    }
};

void A::HiddenSharedPointer::foo()
{
    std::shared_ptr<A>(*this)->foo();
}

template<typename... Args>
A::HiddenSharedPointer::HiddenSharedPointer(Args&&... args)
    : std::shared_ptr<A>(new A(std::forward<Args>(args)...))
{}

std::list<std::pair<A::HiddenSharedPointer, int>> m_ok;

int main()
{
    A::HiddenSharedPointer temp = A::create();
    temp.foo();
    //auto plain_pointer_to_object = temp.get(); // does not compile
    m_ok.push_back(std::pair<A::HiddenSharedPointer, int>(temp, 10));

    temp.reset();

    return 0;
}

Note that I changed the map to a list of pairs, because you would have to provide the operator< for the HiddenSharedPointer class if using a map. Anyway, it's not a good idea to use a (shared) pointer as the key of a map as this imposes undeterministic behavior (i. e. in each run, your map has a different order).

You could implement `operator ->` to return the underlying pointer, but immediately reapplies `->` on it. This is less work that shadowing every method, but I accept that it does leave room for a `GetThis()` method, that would expose the actual raw pointer. `GetThis()` any seem silly, but `GetAsAThingy()` is a common way to avoid the cost of `dynamic_cast<>`. — Gem Taylor, Jul 09 '19 at 16:18

How to avoid dangling pointer with shared_ptr?

2 Answers2