[thread] thread_specific_ptr, dangerous conflation of identity and address

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

Zitat von Edd Dawson <lists@mr-edd.co.uk>:
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!
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?
I have reported this bug here, including a simpler test case: https://svn.boost.org/trac/boost/ticket/3837 and posted a (more efficient) implementation of thread_specific_ptr which doesn't have this bug here: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=tss.hpp&directory=& no reaction from the Boost.Thread maintainer so far. (it's a prototype: it only supports GCC so far, and IIRC I have fixed 1 or 2 bugs locally only, without uploading it. so be careful using it.)

----- Original Message ----- From: <strasser@uni-bremen.de> To: <boost@lists.boost.org> Sent: Monday, February 15, 2010 11:54 PM Subject: Re: [boost] [thread] thread_specific_ptr, dangerous conflation of identity and address
Zitat von Edd Dawson <lists@mr-edd.co.uk>:
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!
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?
I have reported this bug here, including a simpler test case: https://svn.boost.org/trac/boost/ticket/3837
and posted a (more efficient) implementation of thread_specific_ptr which doesn't have this bug here: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=tss.hpp&directory=&
no reaction from the Boost.Thread maintainer so far.
Hi, I find this a rather severe design bug. I will propose you to change the severity to showstopper as we can not use thread_specific_ptr other than statically. Anthony, what do you think? Vicente

"vicente.botet" <vicente.botet@wanadoo.fr> writes:
----- Original Message ----- From: <strasser@uni-bremen.de> To: <boost@lists.boost.org> Sent: Monday, February 15, 2010 11:54 PM Subject: Re: [boost] [thread] thread_specific_ptr, dangerous conflation of identity and address
Zitat von Edd Dawson <lists@mr-edd.co.uk>:
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!
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?
I find this a rather severe design bug. I will propose you to change the severity to showstopper as we can not use thread_specific_ptr other than statically. Anthony, what do you think?
I think that using a thread_specific_ptr as an automatic variable is incorrect usage. The lifetime of the variable is strictly limited to the duration of the function call, and each thread that calls the function will get its own copy of the thread_specific_ptr. Under such circumstances you really ought to just use a plain ordinary variable. It is primarily intended for use with static storage duration. Unfortunately I haven't had a chance to look at the proposed changes. I'll try and get to it today. Anthony -- Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/ just::thread C++0x thread library http://www.stdthread.co.uk Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976

Zitat von Anthony Williams <anthony.ajw@gmail.com>:
I find this a rather severe design bug. I will propose you to change the severity to showstopper as we can not use thread_specific_ptr other than statically. Anthony, what do you think?
I think that using a thread_specific_ptr as an automatic variable is incorrect usage. The lifetime of the variable is strictly limited to the duration of the function call, and each thread that calls the function will get its own copy of the thread_specific_ptr.
you're probably referring to Edd's test case. it's only that, a test case, the bug can be triggered in other scenarios, including thread_specific_ptrs being allocated at the same address on the heap, where multiple threads have access to it. it might be unlikely but it is pretty much unpredictable and depending on the platforms heap allocator, which is never a good thing.
It is primarily intended for use with static storage duration.
if only static usage is intended you might be interested in the static_thread_specific_ptr I use here, whose interface enforces static usage: https://svn.boost.org/svn/boost/sandbox/transaction/boost/transact/detail/st...

On 2/16/2010 8:44 AM, Anthony Williams wrote:
I think that using a thread_specific_ptr as an automatic variable is incorrect usage. The lifetime of the variable is strictly limited to the duration of the function call, and each thread that calls the function will get its own copy of the thread_specific_ptr. Under such circumstances you really ought to just use a plain ordinary variable.
If this is the case then the documentation really ought to say this to set a precedent, I think. However, is it not so far fetched to imagine somebody calling a "big" function of library A, which has a thread_specific_ptr in auto storage and then afterwards calling a "big" function in a completely unrelated library B, which also allocates an auto thread_specific_ptr? I personally hit the problem when running a parallel algorithm where each thread needed to modify a data structure. I created a wrapper around thread_specific_ptr that allowed each thread to have its own copy of a "prototype" data structure to work on: ThreadLocal<vector<Data> > tlData(defaultVector); ParallelForEach(threadPool, input.begin(), input.end(), bind(&ModifyArbitraryElements, _1, Ref(tlData)); // Ref() is like boost::ref() but models a reference to // thread-local data. After all threads were finished, a quick serial pass then merged the data for the different threads together: BOOST_FOREACH(const std::vector<Data> &dataVec, tlData) { MergeData(defaultVector, dataVec); } Of course, running this parallel algorithm multiple times is what caused problems. Since the thread-local data was only needed for the duration of the algorithm, the use of automatic storage seemed rather sensible. But of course I /would/ say that :) Kind regards, Edd

On 2/15/2010 10:54 PM, strasser@uni-bremen.de wrote:
I have reported this bug here, including a simpler test case: https://svn.boost.org/trac/boost/ticket/3837
Good to know, thanks. Much simpler, indeed :)
and posted a (more efficient) implementation of thread_specific_ptr which doesn't have this bug here: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=tss.hpp&directory=&
no reaction from the Boost.Thread maintainer so far.
I hope it is addressed soon, else I'm going to have to end up rolling my own (I need MSVC support, too). Kind regards, Edd
participants (4)
-
Anthony Williams
-
Edd Dawson
-
strasser@uni-bremen.de
-
vicente.botet