Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Bug 699134 - Protect against reentrancy in the LdrLoadDll hook, with …

…less XPCOM stringery and more locking. a=bsmedberg ON A CLOSED TREE

--HG--
extra : transplant_source : F%3Bl%BD%EF%7CM66YP%EA%7E7%60%0C%84%10%22%B0
  • Loading branch information...
commit 435b391163d6743b1e9d96a8ea208e0490f86bb9 1 parent 4c48703
Benjamin Smedberg authored
Showing with 70 additions and 0 deletions.
  1. +70 −0 toolkit/xre/nsWindowsDllBlocklist.cpp
View
70 toolkit/xre/nsWindowsDllBlocklist.cpp
@@ -39,6 +39,9 @@
#include <winternl.h>
#include <stdio.h>
+#include <string.h>
+
+#include <map>
#ifdef XRE_WANT_DLL_BLOCKLIST
#define XRE_SetupDllBlocklist SetupDllBlocklist
@@ -148,10 +151,68 @@ static DllBlockInfo sWindowsDllBlocklist[] = {
// define this for very verbose dll load debug spew
#undef DEBUG_very_verbose
+namespace {
+
typedef NTSTATUS (NTAPI *LdrLoadDll_func) (PWCHAR filePath, PULONG flags, PUNICODE_STRING moduleFileName, PHANDLE handle);
static LdrLoadDll_func stub_LdrLoadDll = 0;
+/**
+ * Some versions of Windows call LoadLibraryEx to get the version information
+ * for a DLL, which causes our patched LdrLoadDll implementation to re-enter
+ * itself and cause infinite recursion and a stack-exhaustion crash. We protect
+ * against reentrancy by allowing recursive loads of the same DLL.
+ *
+ * Note that we don't use __declspec(thread) because that doesn't work in DLLs
+ * loaded via LoadLibrary and there can be a limited number of TLS slots, so
+ * we roll our own.
+ */
+class ReentrancySentinel
+{
+public:
+ explicit ReentrancySentinel(const char* dllName)
+ {
+ DWORD currentThreadId = GetCurrentThreadId();
+ EnterCriticalSection(&sLock);
+ mPreviousDllName = (*sThreadMap)[currentThreadId];
+
+ // If there is a DLL currently being loaded and it has the same name
+ // as the current attempt, we're re-entering.
+ mReentered = mPreviousDllName && !stricmp(mPreviousDllName, dllName);
+ (*sThreadMap)[currentThreadId] = dllName;
+ LeaveCriticalSection(&sLock);
+ }
+
+ ~ReentrancySentinel()
+ {
+ DWORD currentThreadId = GetCurrentThreadId();
+ EnterCriticalSection(&sLock);
+ (*sThreadMap)[currentThreadId] = mPreviousDllName;
+ LeaveCriticalSection(&sLock);
+ }
+
+ bool BailOut() const
+ {
+ return mReentered;
+ };
+
+ static void InitializeStatics()
+ {
+ InitializeCriticalSection(&sLock);
+ sThreadMap = new std::map<DWORD, const char*>;
+ }
+
+private:
+ static CRITICAL_SECTION sLock;
+ static std::map<DWORD, const char*>* sThreadMap;
+
+ const char* mPreviousDllName;
+ bool mReentered;
+};
+
+CRITICAL_SECTION ReentrancySentinel::sLock;
+std::map<DWORD, const char*>* ReentrancySentinel::sThreadMap;
+
static NTSTATUS NTAPI
patched_LdrLoadDll (PWCHAR filePath, PULONG flags, PUNICODE_STRING moduleFileName, PHANDLE handle)
{
@@ -239,6 +300,11 @@ patched_LdrLoadDll (PWCHAR filePath, PULONG flags, PUNICODE_STRING moduleFileNam
#endif
if (info->maxVersion != ALL_VERSIONS) {
+ ReentrancySentinel sentinel(dllName);
+ if (sentinel.BailOut()) {
+ goto continue_loading;
+ }
+
// In Windows 8, the first parameter seems to be used for more than just the
// path name. For example, its numerical value can be 1. Passing a non-valid
// pointer to SearchPathW will cause a crash, so we need to check to see if we
@@ -307,11 +373,15 @@ patched_LdrLoadDll (PWCHAR filePath, PULONG flags, PUNICODE_STRING moduleFileNam
WindowsDllInterceptor NtDllIntercept;
+} // anonymous namespace
+
void
XRE_SetupDllBlocklist()
{
NtDllIntercept.Init("ntdll.dll");
+ ReentrancySentinel::InitializeStatics();
+
bool ok = NtDllIntercept.AddHook("LdrLoadDll", reinterpret_cast<intptr_t>(patched_LdrLoadDll), (void**) &stub_LdrLoadDll);
#ifdef DEBUG
Please sign in to comment.
Something went wrong with that request. Please try again.