Segmentation fault in destructor of own shared pointer

Question

For training purposes, I am trying to write my own smartpointer, imitating std::shared_ptr. I have a static std::map<void *, int> ref_track that keeps track whether there are still shared pointer referencing a certain block in memory.

My concept is this:

template <typename PType>
class shared_ptr
{
    public:
        shared_ptr()
        : value_(nullptr), ptr_(nullptr)
        {}

        template <typename T>
        explicit shared_ptr(T * ptr)
        : shared_ptr()
        {
            reset(ptr);
        }

        template <typename T>
        shared_ptr(shared_ptr<T> const & other)
        : shared_ptr()
        {       
            reset(other.get());
        }

        ~shared_ptr()
        {
            reset();
        }

        void reset()
        {
            if(value_)
            {
                delete value_; // Segmentation fault here!
                value_ = 0;
                ptr_ = 0;
            }
        }

        template <typename T>
        void reset(T * ptr)
        {   
            reset();
            if(ptr)
            {
                value_ = new shared_ptr_internal::storage_impl<
                    T
                >(ptr);
                ptr_ = ptr;
            }
        }

        PType * get() const
        {
            return ptr_;
        }

        typename shared_ptr_internal::ptr_trait<PType>::type operator *()
        {
            return *ptr_;
        }

    private:
        shared_ptr_internal::storage_base * value_;
        PType * ptr_;
};

When running my test suite, I noticed that

shared_ptr<int> a(new int(42));
a.reset(new int(13));

works fine, but

shared_ptr<int> a(new int(42));
a = shared_ptr<int>(new int(13));

leads to problems: *a is 0 instead of 13, and delete value_ crashes with a segmentation fault in the destructor of a. I have marked the crash in the source code with a comment.

The used internal classes are

namespace shared_ptr_internal
{

    typedef std::map<void *, int> ref_tracker;
    typedef std::map<void *, int>::iterator ref_tracker_iterator;
    typedef std::pair<void *, int> ref_tracker_entry;

    static ref_tracker ref_track;

    struct storage_base
    {
        virtual ~storage_base() {}
    };

    template <typename PType>
    struct storage_impl : storage_base
    {
        storage_impl(PType * ptr)
        : ptr_(ptr)
        {
            ref_tracker_iterator pos = ref_track.find(ptr);
            if(pos == ref_track.end())
            {
                ref_track.insert(
                    ref_tracker_entry(ptr, 1)
                );
            }
            else
            {
                ++pos->second;
            }
        }

        ~storage_impl()
        {
            ref_tracker_iterator pos = ref_track.find(ptr_);
            if(pos->second == 1)
            {
                ref_track.erase(pos);
                delete ptr_;
            }
            else
            {
                --pos->second;
            }
        }

        private:
            PType * ptr_;
    };

    template <typename PType>
    struct ptr_trait
    {
        typedef PType & type;
    };

    template <>
    struct ptr_trait<void>
    {
        typedef void type;
    };
}

Sorry for the bulk of source code, but I really do not know how I could narrow it down further. I would be grateful for any ideas what could be causing the segfault, and moreover why this does not happen when using reset manually.

Update

My (not-working) assignment operator:

template <typename T>
shared_ptr<PType> & operator =(shared_ptr<T> const & other)
{
    if(this != &other)
    {
        value_ = nullptr;
        ptr_ = nullptr;
        reset(other.get());
    }
    return *this;
}

"I have a static std::map ref_track that keeps track" that seems like a bad idea, and unneeded — stijn, Jul 02 '12 at 11:09
Offtopic: `template shared_ptr(shared_ptr const & other) : shared_ptr() { reset(other.get()); } ` should not compile. You call a constructor from another one? — Ferenc Deak, Jul 02 '12 at 11:14
Not to mention an `O(log(N))` lookup time. Hash table would be a better idea. — Puppy, Jul 02 '12 at 11:14
@stijn I don't think it's unneeded, because 1) different instances may be referencing the same memory block, so I need something static to keep track of that, and 2) `shared_ptr` and `shared_ptr` could be referencing the same memory block, so a static map within the class would not work. Could you elaborate why you think it is not needed? — nikolas, Jul 02 '12 at 11:14
@DeadMG Thanks :) I need to update my compiler :D and my brain ... — Ferenc Deak, Jul 02 '12 at 11:15
@nijansen I'm not 100% sure, but no implementations of shared_ptr I know use a static map, instead they use a simple local refcounter instance shared amongst instances having the same pointer — stijn, Jul 02 '12 at 11:21

score 2 · Accepted Answer · answered Jul 02 '12 at 11:08

2

You're missing an assignment operator.

This means that in the following code:

a = shared_ptr<int>(new int(13));

a temporary shared pointer is created; then the default assignment operator simply copies the pointer to a without releasing the old value or updating the reference count; then the temporary deletes the value, leaving a with a dangling pointer.

answered Jul 02 '12 at 11:08

Mike Seymour

249,747
28
448
644

I tried to add an assignment operator (I have updated my post with my approach for proper code formatting), but it does not fix the problem. I've added a debug breakpoint to the function, but the breakpoint is never even reached. What am I doing wrong? – nikolas Jul 02 '12 at 12:37
@nijansen: Setting `value_` and `ptr_` to null is certainly wrong; it means the reference count for the old object won't be decremented. Just `if (this != &other) reset(other.get());` should work, as long as the strange reference counting works. The map-based reference counting is too weird for me to debug in my head; the best thing would be to step through it in a debugger, and see exactly when things are being deleted. – Mike Seymour Jul 02 '12 at 12:45

Grizzly · Answer 2 · 2012-07-02T11:16:19.847

Seems like a violation of the rule of three: You have a custom copy constructor and a custom destructor, but no custom assignment operator. Therefore a = shared_ptr<int>(new int(13)) will just copy the two pointers value_ and ptr_ from the temporary, without any update of your reference tracking. Therefore when you destroy the temporary, there are no more tracked references to that pointer, which will lead to its deletion. Also note that the old pointer will have been leaked in the assignment.

score 0 · Answer 3 · answered Jul 02 '12 at 11:11

you forgot to add an assignment operator to your pointer class that should decrement the number references to the old object and increment the number of references to the assigned object. Most times it's the easiest way to implement operator= in terms of a copy d'tor and a swap function:

shared_ptr& shared_ptr<T>::operator=( shared_ptr<T> other ) 
{
    other.swap( this );

    return *this;
}

Segmentation fault in destructor of own shared pointer

3 Answers3