[reSIProcate] [PATCH] Windows Thread Local Storage API issue

[Aron Rosenberg <arosenberg@xxxxxxxxxxxx>]

The following patch fixes an error in the API usage of the Windows Thread Local Storage backend that was found with the Microsoft Application Verifier (http://www.microsoft.com/download/en/details.aspx?id=20028). The main problem is that while the maximum number of TLS slots are limited to 1088, the value of that index does not have to be 0->1087, it can be any DWORD value. The patch changes ThreadIf to use a std::map to store the TLS key's instead of a fixed array so no possible heap corruption can occur if index is > 1088. With the patch applied, there are no other AppVerifier errors reported in testStack.

TLS was introduced in rev 8672, 11-2009 (committed by Scott, patch from Alexander Chemeris). https://svn.resiprocate.org/viewsvn/resiprocate?view=revision&revision=8672

Will commit this sometime this week unless I hear from anybody.

-Aron

Aron Rosenberg

Sr. Director, Engineering,

LifeSize, a division of Logitech

Index: rutil/ThreadIf.hxx

==================================================================

--- rutil/ThreadIf.hxx (revision 9233)

+++ rutil/ThreadIf.hxx (working copy)

@@ -6,6 +6,7 @@

 #ifdef WIN32

 #  include <BaseTsd.h>

 #  include <winbase.h>

+#  include <map>

 #else

 #  include <pthread.h>

 #endif

@@ -98,15 +99,13 @@

    protected:

 #ifdef WIN32

       HANDLE mThread;

-      /// 1088 is the maximum number of TLS slots under Windows.

-      /// Refer to http://msdn.microsoft.com/en-us/library/ms686749(VS.85).aspx

-      enum {TLS_MAX_KEYS=1088};

+      typedef std::map<DWORD,TlsDestructor *> TlsDestructorMap;

    public:

       /// Free data in TLS slots. For internal use only!

       static void tlsDestroyAll();

    protected:

-      /// Array of TLS destructors. We have to emulate TLS destructors under Windows.

-      static TlsDestructor **mTlsDestructors;

+      /// Map of TLS destructors. We have to emulate TLS destructors under Windows.

+      static TlsDestructorMap *mTlsDestructors;

       /// Mutex to protect access to mTlsDestructors

       static Mutex *mTlsDestructorsMutex;

       friend class TlsDestructorInitializer;

Index: rutil/ThreadIf.cxx

===================================================================

--- rutil/ThreadIf.cxx (revision 9233)

+++ rutil/ThreadIf.cxx (working copy)

@@ -28,7 +28,7 @@

 using namespace resip;

 

 #ifdef WIN32

-ThreadIf::TlsDestructor **ThreadIf::mTlsDestructors;

+ThreadIf::TlsDestructorMap *ThreadIf::mTlsDestructors;

 Mutex *ThreadIf::mTlsDestructorsMutex;

 #endif

 

@@ -69,11 +69,7 @@

    if (mInstanceCounter++ == 0)

    {

       ThreadIf::mTlsDestructorsMutex = new Mutex();

-      ThreadIf::mTlsDestructors = new ThreadIf::TlsDestructor*[ThreadIf::TLS_MAX_KEYS];

-      for (int i=0; i<ThreadIf::TLS_MAX_KEYS; i++)

-      {

-         ThreadIf::mTlsDestructors[i] = NULL;

-      }

+      ThreadIf::mTlsDestructors = new ThreadIf::TlsDestructorMap;

    }

 }

 TlsDestructorInitializer::~TlsDestructorInitializer()

@@ -81,7 +77,8 @@

    if (--mInstanceCounter == 0)

    {

       delete ThreadIf::mTlsDestructorsMutex;

-      delete [] ThreadIf::mTlsDestructors;

+      ThreadIf::mTlsDestructors->clear();

+      delete ThreadIf::mTlsDestructors;

    }

 }

 #endif

@@ -231,7 +228,7 @@

    if (key!=TLS_OUT_OF_INDEXES)

    {

       Lock lock(*mTlsDestructorsMutex);

-      mTlsDestructors[key] = destructor;

+      (*mTlsDestructors)[key] = destructor;

       return 0;

    }

    else

@@ -250,7 +247,7 @@

    if (TlsFree(key)>0)

    {

       Lock lock(*mTlsDestructorsMutex);

-      mTlsDestructors[key] = NULL;

+      mTlsDestructors->erase(key);

       return 0;

    }

    else

@@ -318,16 +315,15 @@

 ThreadIf::tlsDestroyAll()

 {

    Lock lock(*mTlsDestructorsMutex);

-   for (int i=0; i<TLS_MAX_KEYS; i++)

+   ThreadIf::TlsDestructorMap::const_iterator i = mTlsDestructors->begin();

+   while(i != mTlsDestructors->end())

    {

-      if (mTlsDestructors[i] != NULL)

+      void *val = TlsGetValue(i->first);

+      if (val != NULL)

       {

-         void *val = TlsGetValue(i);

-         if (val != NULL)

-         {

-            (*mTlsDestructors[i])(val);

-         }

+         (*(i->second))(val);

       }

+      i++;

    }

 }

 #endif