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

[Aron Rosenberg]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20110717/e68bd1af/attachment.htm>