Opened 16 years ago
Closed 16 years ago
#1979 closed defect (fixed)
CPLSPrintf is not thread safe
Reported by: | daniel112b | Owned by: | warmerdam |
---|---|---|---|
Priority: | high | Milestone: | 1.5.3 |
Component: | default | Version: | unspecified |
Severity: | normal | Keywords: | threadsafety |
Cc: | daniel112b@… |
Description
It looks like CPLSPrintf is using thread local storage for it's buffers but it's not. CPL_THREADLOCAL is just defined to be empty.
My problems occur while i'm trying to extract the styling info and i end up with an empty style string when running multiple threads that read data from their own OGRDataSource.
http://lists.maptools.org/pipermail/gdal-dev/2007-November/014787.html
Attachments (2)
Change History (12)
comment:1 by , 16 years ago
Keywords: | threadsafety added |
---|---|
Milestone: | → 1.4.4 |
Status: | new → assigned |
comment:2 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:3 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
This change in r12686 causes problems when the encapsulated string is returned (as const char*), and the encapsulated string is deallocated when the CPLString goes out of scope. For more information about this issue see the following mapserver ticket:
comment:4 by , 16 years ago
I confirm Tamas' analysis.
The change of CPLSPrintf to CPLString().Printf() is wrong, at least when the string is used as a return value.
See the attached test case. Valgrind complains loudly. Crashes may happen.
by , 16 years ago
Test case to understand correct/bad CPLString().Printf() usage
comment:5 by , 16 years ago
Even,
Small addition to the test case, CPLString can be passed to foo() only becuase there is conversion operator provided (line 14) in the CPLString class.
The bar() is ill-formed, according to semantic of CPLString::Printf. The semantic of the Printf is the same as of functions like std::string::c_str() or std::string::data(), so this is invalid from the same reasons:
const char* goo() { return std::string("abc").c_str(); }
follow-up: 7 comment:6 by , 16 years ago
Milestone: | 1.4.4 → 1.5.3 |
---|---|
Priority: | normal → high |
A grep on "return CPLString" shows ogr/ogrfeaturestyle.cpp and frmts/wms/stuff.cpp, but the later is not concerned by the bug.
So the following patch should be enough (not tested). To be applied in branches/1.5 too as the change was done during 1.5 development.
Index: ogr/ogrfeaturestyle.cpp =================================================================== --- ogr/ogrfeaturestyle.cpp (révision 15119) +++ ogr/ogrfeaturestyle.cpp (copie de travail) @@ -1495,19 +1495,19 @@ return sStyleValue.pszValue; case OGRSTypeDouble: if (sStyleParam.bGeoref) - return CPLString().Printf("%f",ComputeWithUnit(sStyleValue.dfValue, + return CPLSPrintf("%f",ComputeWithUnit(sStyleValue.dfValue, sStyleValue.eUnit)); else - return CPLString().Printf("%f",sStyleValue.dfValue); + return CPLSPrintf("%f",sStyleValue.dfValue); case OGRSTypeInteger: if (sStyleParam.bGeoref) - return CPLString().Printf("%d",ComputeWithUnit(sStyleValue.nValue, + return CPLSPrintf("%d",ComputeWithUnit(sStyleValue.nValue, sStyleValue.eUnit)); else - return CPLString().Printf("%d",sStyleValue.nValue); + return CPLSPrintf("%d",sStyleValue.nValue); case OGRSTypeBoolean: - return CPLString().Printf("%d",sStyleValue.nValue); + return CPLSPrintf("%d",sStyleValue.nValue); default: bValueIsNull = TRUE; return NULL;
comment:7 by , 16 years ago
It must be noted, that this patch revokes the corresponding change in r12686 to it's original state. And it has been denoted as a thread unsafe approach...
comment:8 by , 16 years ago
The original implementation of CPLSPrintf was thread unsafe, but not the current implementation that uses TLS. See r12681
comment:10 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Problem confirmed. The CPLSPrintf() buffer ring needs to be made thread local using the CPL*TLS services. I will take care of this issue.