CPLSPrintf is not thread safe

[daniel112b]

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

[warmerdam]

Problem confirmed. The CPLSPrintf() buffer ring needs to be made thread local using the CPL*TLS services. I will take care of this issue.

[warmerdam]

Fixed in trunk as r12681 as r12683. Ported to 1.4 branch as r12688.

Also applied a big pass through trunk removing use of CPLSPrintf() and replacing it with CPLString based solutions as r12686. Substantial work still to do on this as time permits.

[tamas]

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:

 http://trac.osgeo.org/mapserver/ticket/2656

[rouault]

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.

[rouault]

Test case to understand correct/bad CPLString().Printf() usage

[rouault]

Output of Valgrind execution on test program

[mloskot]

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

[rouault]

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;

[tamas]

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...

[rouault]

The original implementation of CPLSPrintf was thread unsafe, but not the current implementation that uses TLS. See r12681

[tamas]

Tested the patch and seems OK.

Applied in r15124 and r15125