[Threads-devel] Potential memory leak with thread_specific_ptr

Subject: [Threads-devel] Potential memory leak with thread_specific_ptr
From: Chris Newbold (Chris.Newbold_at_[hidden])
Date: 2012-09-10 09:39:01

Greetings. I believe I have found a potential memory leak with thread_specific_ptr. The potential leak occurs if an instance of thread_specific_ptr is created and then destroyed without ever being assigned to. Consider the following pieces of code, excerpted from the Boost.Thread library in release 1.49:

>From tss.hpp:

        thread_specific_ptr():
            cleanup(detail::heap_new<delete_data>(),detail::do_heap_delete<delete_data>())
        {}

        ~thread_specific_ptr()
        {
            detail::set_tss_data(this,boost::shared_ptr<detail::tss_cleanup_function>(),0,true);
        }

There are only two places thread_specific_ptr calls set_tss_data: the destructor and in reset(). So if an instance of thread_specific_ptr is never used during its lifetime, the first call to set_tss_data will occur from the destructor.

Now, looking at pthread/thread.cpp shows that set_tss_data has special handling for the combination of an empty cleanup function and a zero tss value which erases the corresponding node. But that handling is inside of a test for an existing node. Otherwise, a new node is unconditionally allocated. This is where I believe the leak originates.

I think the tests in set_tss_data need to be rearranged in order to avoid gratuitously creating a new tss node when unused thread_specific_ptr instances are destroyed.

        void set_tss_data(void const* key,
                          boost::shared_ptr<tss_cleanup_function> func,
                          void* tss_data,bool cleanup_existing)
        {
            if(tss_data_node* const current_node=find_tss_data(key))
            {
                if(cleanup_existing && current_node->func && (current_node->value!=0))
                {
                    (*current_node->func)(current_node->value);
                }
                if(func || (tss_data!=0))
                {
                    current_node->func=func;
                    current_node->value=tss_data;
                }
                else
                {
                    erase_tss_node(key);
                }
            }
            else
            {
                add_new_tss_node(key,func,tss_data);  // <<<<<< LEAK OCCURS HERE!
            }
        }

Though the implementation of set_tss_data in win32/thread.cpp is structured somewhat differently, I believe it suffers the same problem

-Chris