race condition in static variable bTLSKeySetup Cpl_multiproc.cpp

[lucafibbi]

In the Cpl_multiproc.cpp file when using the static variable bTLSKeySetup simultaneously from multiple threads race condition occurs.

I think that the following implementation of the functions CPLCleanupTLS and CPLGetTLSList for pthred library should be thread safe: {{ static pthread_once_t oTLSKeySetup = PTHREAD_ONCE_INIT;

static void CPLMake_key() {

if( pthread_key_create( &oTLSKey,

(void (*)(void*)) CPLCleanupTLSList ) != 0 )

{

CPLError( CE_Fatal, CPLE_AppDefined,

"pthread_key_create() failed!" );

}

}

/************************************************************************/ /* CPLCleanupTLS() */ /************************************************************************/

void CPLCleanupTLS()

{

void **papTLSList;

papTLSList = (void **) pthread_getspecific( oTLSKey ); if( papTLSList == NULL )

return;

pthread_setspecific( oTLSKey, NULL );

CPLCleanupTLSList( papTLSList );

}

/************************************************************************/ /* CPLGetTLSList() */ /************************************************************************/

static void **CPLGetTLSList()

{

void **papTLSList;

if ( pthread_once(&oTLSKeySetup, CPLMake_key) != 0 ) {

CPLError( CE_Fatal, CPLE_AppDefined,

"pthread_once() failed!" );

}

papTLSList = (void **) pthread_getspecific( oTLSKey ); if( papTLSList == NULL ) {

papTLSList = (void **) CPLCalloc(sizeof(void*),CTLS_MAX*2); if( pthread_setspecific( oTLSKey, papTLSList ) != 0 ) {

CPLError( CE_Fatal, CPLE_AppDefined,

"pthread_setspecific() failed!" );

}

}

return papTLSList;

} }}}

[warmerdam]

Ivan,

Could you review and if appropriate apply the changes?

[ilucena]

Frank,

I applied those changes (on my local files) but I am not sure how can I test it.

[warmerdam]

Ivan,

Generally speaking you will not be able to trigger such a race condition since the impact is so rare. But you should ensure you are built with pthread support on, and perhaps run the test suite.

[ilucena]

Frank,

Does the builtbot setting include multi-threads support in Linux and Windows?

Is there a specific test case for multi-threads?

r19918

[rouault]

Ivan,

from what I see ( http://buildbot.osgeo.org:8500/builders/telascience-full/builds/518/steps/configure/logs/stdio), the Linux slavebots are not configured with --with-threads. (The windows builds of GDAL have always threading turned on however)

test_gdalwarp_18() in autotest/utilities/test_gdalwarp.py will use the -multi option of gdalwarp. Apart from that one, I don't think there are any other test that make use of multi-threading. And I'm 99,9999% sure that it wouldn't exhibit the race condition of that ticket.

The race condition that existed before would have been very hard to reproduce. It could happen only the first time CPLGetTLSList() was called. Basically, you'd need to make a special program that has 2 threads calling CPLGetTLS(). As often with multi-threading issues, only careful code review can really tell if the code is correct or not.

I'd note that the Win32 implementation of CPLGetTLSList() is still subject to a similar race condition that the pthread implementation was. According to  http://msdn.microsoft.com/en-us/library/ms885202.aspx, TlsAlloc?() would be best called frm a DLL entry point to avoid race conditions. But that'd a different story/ticket