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

[Scott Godin]

Nice find - thanks Aron!

Scott

On Mon, Jul 18, 2011 at 12:50 AM, Aron Rosenberg <arosenberg at logitech.com>wrote:

> 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
>
>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20110724/00fbe519/attachment.htm>