Re: [boost] [thread] patch for tss.cpp (win32 + static linkingsupport)

From: Anthony Williams (anthony_w.geo_at_[hidden])
Date: 2007-05-03 05:59:06

"Johan Nilsson" <r.johan.nilsson_at_[hidden]> writes:

> Anthony Williams wrote:
>> "Johan Nilsson" <r.johan.nilsson_at_[hidden]> writes:
>>
>>> I believe I've found a bug in tss.cpp, for details please ref
>>> http://article.gmane.org/gmane.comp.lib.boost.user/26591 .
>>>
>>> The immediate fix is straightforward, it just involves resetting the
>>> native tss value after deleting the pointed-to data at thread exit.
>>> Patch is attached to this message. The changes only affects the
>>> Win32 platform and I've run the supplied tests under
>>> libs/thread/test to verify that it doesn't break anything obvious.
>>>
>>> As the bug involves dereferencing a dangling pointer and isn't
>>> detected most of the time, I'd really appreciate if the patch could
>>> be accepted as soon as possible. I'm awaiting Roland Schwartz to
>>> "bless" the patch. If he agrees, would it be possible to accept this
>>> for 1.34?
>>
>> Your patch calls TlsSetValue after calling cleanup_slots.
>> cleanup_slots may call TlsFree, in which case this is not a safe
>> thing to do. A correct fix would require further investigation.

> - One solution would be to statically initialize tss_data_native_key to
> TLS_OUT_OF_INDEXES, and reset to that value from tss_data_dec_use in the
> above case. The code in tss_thread_exit could then check for a valid tls
> index before calling TlsSetValue.
>
> There shouldn't be a race condition checking/manipulating the native tls
> index as it should exist only one thread using tss data at this point ... 
> or? Am I missing something again?

There isn't a race condition per-se, but there is no logic to reinitialize
tss_data_native_key once it has been freed, so that would have to be added.

> - A second alternative would be to remove the TlsFree call altogether, as
> the application is shutting down anyway. A bit dirty perhaps.

Possible, yes --- the code to call TlsFree is relatively new. Dirty, yes. I
think the TlsFree call should be moved to on_process_exit, but that's not a
trivial change. You'd still need to call TlsSetValue to clear the entry.

> Do you have any suggestions for a better fix? If it would be possible to
> have tss_thread_exit called at the correct time for the main thread, the
> problems would probably be nonexistent.

Are you using Boost.Thread as a DLL or a LIB file? If you're using it as a lib
file, then the code in tss_pe.cpp controls when the exit stuff is called.

It is possible to make on_thread_exit be called correctly for the main thread,
by calling it on process exit, but the problem here is that at this point in
the process shutdown, things like new and delete don't necessarily work
correctly, and the runtime has already been shut down --- the TSS code would
need to be changed to use HeapAlloc/HeapFree rather than new and delete, and
then there's the problem of what the destructors for the TSS data do: they
can't necessarily use any library functions either, and that's not enforceable
or readily explainable.

Having global destructors and atexit functions use TSS is a bad idea; this is
just a manifestation of that.

Sorry, but I don't think this can be fixed for 1.34.0.

Anthony
-- 
Anthony Williams
Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk
Registered in England, Company Number 5478976.
Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL