CODEWITHSUNDEEP
c++segmentation-faultshared-ptrsmart-pointers
nikolas
nikolas

Reputation: 8975

Segmentation fault in destructor of own shared pointer

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;
}

Upvotes: 2

Views: 4147

Answers (3)

Grizzly
Grizzly

Reputation: 20191

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.

Upvotes: 2

Torsten Robitzki
Torsten Robitzki

Reputation: 2555

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;
}

Upvotes: 0

Mike Seymour
Mike Seymour

Reputation: 254431

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.

Upvotes: 2

Related Questions