Opened 11 years ago

Closed 11 years ago

#3628 closed defect (fixed)

race condition in static variable bTLSKeySetup Cpl_multiproc.cpp

Reported by: lucafibbi Owned by: ilucena
Priority: normal Milestone: 1.8.0
Component: default Version: unspecified
Severity: normal Keywords: thread safe
Cc: warmerdam

Description (last modified by warmerdam)

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;

} }}}

Change History (5)

comment:1 Changed 11 years ago by warmerdam

Cc: warmerdam added
Description: modified (diff)
Milestone: 1.8.0
Owner: changed from warmerdam to ilucena

Ivan,

Could you review and if appropriate apply the changes?

comment:2 Changed 11 years ago by ilucena

Frank,

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

comment:3 Changed 11 years ago by 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.

comment:4 Changed 11 years ago by ilucena

Frank,

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

Is there a specific test case for multi-threads?

r19918

comment:5 Changed 11 years ago by Even Rouault

Milestone: 1.8.0
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.