Opened 12 years ago

Closed 11 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)

test.cpp (1.1 KB) - added by Even Rouault 11 years ago.
Test case to understand correct/bad CPLString().Printf() usage
test.out (5.9 KB) - added by Even Rouault 11 years ago.
Output of Valgrind execution on test program

Download all attachments as: .zip

Change History (12)

comment:1 Changed 12 years ago by warmerdam

Keywords: threadsafety added
Milestone: 1.4.4
Status: newassigned

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

comment:2 Changed 12 years ago by warmerdam

Resolution: fixed
Status: assignedclosed

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.

comment:3 Changed 11 years ago by tamas

Resolution: fixed
Status: closedreopened

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

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

Changed 11 years ago by Even Rouault

Attachment: test.cpp added

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

Changed 11 years ago by Even Rouault

Attachment: test.out added

Output of Valgrind execution on test program

comment:5 Changed 11 years ago by Mateusz Łoskot

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

comment:6 Changed 11 years ago by Even Rouault

Milestone: 1.4.41.5.3
Priority: normalhigh

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 in reply to:  6 Changed 11 years ago by 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...

comment:8 Changed 11 years ago by Even Rouault

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

comment:9 Changed 11 years ago by tamas

Tested the patch and seems OK.

Applied in r15124 and r15125

comment:10 Changed 11 years ago by Even Rouault

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.