[boost] [thread] thread_specific_ptr, dangerous conflation of identity and address

Subject: [boost] [thread] thread_specific_ptr, dangerous conflation of identity and address
From: Edd Dawson (lists_at_[hidden])
Date: 2010-02-15 17:39:22

Hi folks,

A thread_specific_ptr appears to use its own address as a key. For example, the 
two tss objects created below will likely share the same key:

   void f()
   {
       thread_specific_ptr<int> ptr;
       // ...
   }

   int main()
   {
       f();
       f();
   }

When thread_specific_ptr's destructor is called, it does a reset() which clears 
the data for the thread doing the destruction. However, data for other threads 
still remain. I find this asymmetric behaviour a little strange. Why should all 
threads but one see their old data?

The bigger problem though, is that it might not be *their* old data. It can just 
as easily be someone else's old data due to coincidental alignment of different 
thread_specific_ptrs on the stack at entirely unrelated points in the program, 
or even in different libraries that each happen to use boost::thread_specific_ptr!

   void g()
   {
       thread_specific_ptr<float> ptr;
       // ...
   }

   f();
   g();

It's easy to see the problem here as the calls are very close to one another. In 
general, however, this strikes me as being really rather dangerous behaviour.

Would it be fair to call it a bug? It seems it is only safe to allocate 
thread_specific_ptrs in static storage.

A complete example is listed at the end. It exhibits the kind of behaviour I'm 
talking about on my x86 systems, at least.

Kind regards,

Edd

//#define BOOST_ALL_NO_LIB 1

#include <queue>
#include <iostream>
#include <boost/thread/tss.hpp>
#include <boost/thread/thread.hpp>
#include <boost/thread/barrier.hpp>
#include <boost/thread/condition.hpp>
#include <boost/thread/mutex.hpp>
#include <boost/bind.hpp>

using namespace boost;

typedef function<void ()> task_type;

// Simple multi-producer-single-consumer job queue.
// Manages its own consumer thread.
class jobq
{
     public:
         jobq() :
             dying_(false),
             consumer_(bind(&jobq::work_loop, this))
         {
         }

         ~jobq()
         {
             exit_work_loop();
             consumer_.join();
         }

         void add(const task_type &task)
         {
             unique_lock<mutex> lk(mtx_);
             tasks_.push(task);
             arrival_.notify_one();
         }

     private:
         void exit_work_loop()
         {
             unique_lock<mutex> lk(mtx_);
             dying_ = true;
             arrival_.notify_one();
         }

         bool wait_for_task(task_type &task)
         {
             unique_lock<mutex> lk(mtx_);
             while (!dying_ && tasks_.empty())
                 arrival_.wait(lk);

             if (dying_) return false;

             task = tasks_.front();
             tasks_.pop();
             return true;
         }

         void work_loop()
         {
             task_type task;
             while (wait_for_task(task))
                 task();
         }

     private:
         bool dying_;
         std::queue<task_type> tasks_;
         mutex mtx_;
         condition arrival_;
         thread consumer_;
};

template<typename T>
void access_tsp(thread_specific_ptr<T> &tsp, barrier &b)
{
     if (!tsp.get())
         tsp.reset(new T(-1));
     else
         std::cout << "Already initialized to " << *tsp << "!\n";

     b.wait();
}

template<typename T>
void f(jobq &q)
{
     barrier b(2);
     thread_specific_ptr<T> tsp;
     q.add(bind(&access_tsp<T>, ref(tsp), ref(b)));
     b.wait(); // wait until it's safe to destroy tsp
}

int main()
{
     jobq q;

     f<int>(q);
     f<unsigned>(q);
     return 0;
}