Fix AV in FindRealCode and increment version number (#8)

* Fix AV in FindRealCode and increment version number

* Switch back to Release in the setup file

* Addressed CR comments

Author: dcristoloveanu

setup/version.h

@@ -1,9 +1,9 @@
 
-#define VLDVERSION          L"2.5.6"
-#define VERSION_NUMBER		2,5,6,0
-#define VERSION_STRING		"2.5.6.0"
+#define VLDVERSION          L"2.5.7"
+#define VERSION_NUMBER		2,5,7,0
+#define VERSION_STRING		"2.5.7.0"
 #define VERSION_COPYRIGHT	"Copyright (C) 2005-2020"
 
 #ifndef __FILE__
-!define VLD_VERSION "2.5.6"	// NSIS Script
+!define VLD_VERSION "2.5.7"	// NSIS Script
 #endif

setup/vld-setup.iss

@@ -2,7 +2,7 @@
 ; SEE THE DOCUMENTATION FOR DETAILS ON CREATING INNO SETUP SCRIPT FILES!
 
 #define MyAppName "Visual Leak Detector"
-#define MyAppVersion "2.5.6"
+#define MyAppVersion "2.5.7"
 #define MyAppPublisher "VLD Team"
 #define MyAppURL "http://vld.codeplex.com/"
 #define MyAppRegKey "Software\Visual Leak Detector"

src/utility.cpp

@@ -423,38 +423,81 @@ BOOL IsModulePatched (HMODULE importmodule, moduleentry_t patchtable [], UINT ta
 
     // No patches listed in the patch table were found in the import module.
     return FALSE;
-}
-
+}
+
 LPVOID FindRealCode(LPVOID pCode)
-{
-    LPVOID result = pCode;
+{
+    LPVOID result;
     if (pCode != NULL)
     {
-        if (*(WORD *)pCode == 0x25ff) // JMP r/m32
-        {
+        // we need to make sure we can read the first 3 ULONG_PTRs
+        DWORD old_protect;
+        if (VirtualProtect(pCode, sizeof(ULONG_PTR) * 3, PAGE_EXECUTE_READ, &old_protect))
+        {
+            if (*(WORD*)pCode == 0x25ff) // JMP r/m32
+            {
 #ifdef _WIN64
-            LONG offset = *((LONG *)((ULONG_PTR)pCode + 2));
-            // RIP relative addressing
-            PBYTE pNextInst = (PBYTE)((ULONG_PTR)pCode + 6);
-            pCode = *(LPVOID*)(pNextInst + offset);
-            return pCode;
+                LONG offset = *((LONG*)((ULONG_PTR)pCode + 2));
+                // RIP relative addressing
+                PBYTE pNextInst = (PBYTE)((ULONG_PTR)pCode + 6);
+
+                // now that we got the offset, make sure we can read the code at the offset
+                DWORD old_protect_2;
+                PBYTE addr = pNextInst + offset;
+                if (VirtualProtect(addr, sizeof(LPVOID), PAGE_EXECUTE_READ, &old_protect_2))
+                {
+                    result = *(LPVOID*)(addr);
+                    (void)VirtualProtect(addr, sizeof(LPVOID), old_protect_2, &old_protect_2);
+                }
+                else
+                {
+                    result = NULL;
+                }
 #else
-            DWORD addr = *((DWORD *)((ULONG_PTR)pCode + 2));
-            pCode = *(LPVOID*)(addr);
-            return FindRealCode(pCode);
+                DWORD addr = *((DWORD*)((ULONG_PTR)pCode + 2));
+                // now that we got the address to read, make sure we can read the code at the offset
+                DWORD old_protect_2;
+                if (VirtualProtect((LPVOID*)addr, sizeof(LPVOID), PAGE_EXECUTE_READ, &old_protect_2))
+                {
+                    pCode = *(LPVOID*)(addr);
+                    result = FindRealCode(pCode);
+                    (void)VirtualProtect((LPVOID*)addr, sizeof(LPVOID), old_protect_2, &old_protect_2);
+                }
+                else
+                {
+                    result = NULL;
+                }
 #endif
+            }
+            else if (*(BYTE*)pCode == 0xE9) // JMP rel32
+            {
+                // Relative next instruction
+                PBYTE pNextInst = (PBYTE)((ULONG_PTR)pCode + 5);
+                LONG offset = *((LONG*)((ULONG_PTR)pCode + 1));
+                pCode = (LPVOID*)(pNextInst + offset);
+                result = FindRealCode(pCode);
+            }
+            else
+            {
+                result = pCode;
+            }
+
+            // restore the page protection state
+            (void)VirtualProtect(pCode, sizeof(ULONG_PTR) * 3, old_protect, &old_protect);
+        }
+        else
+        {
+            result = NULL;
         }
-        if (*(BYTE *)pCode == 0xE9) // JMP rel32
-        {
-            // Relative next instruction
-            PBYTE	pNextInst = (PBYTE)((ULONG_PTR)pCode + 5);
-            LONG	offset = *((LONG *)((ULONG_PTR)pCode + 1));
-            pCode = (LPVOID*)(pNextInst + offset);
-            return FindRealCode(pCode);
-        }
-    }
+    }
+    else
+    {
+        result = NULL;
+    }
     return result;
 }
+
+#define MAX_PATCH_ENTRY_COUNT 128
 
 // PatchImport - Patches all future calls to an imported function, or references
 //   to an imported variable, through to a replacement function or variable.
@@ -515,106 +558,131 @@ BOOL PatchImport (HMODULE importmodule, moduleentry_t *patchModule)
 	LPSTR pszBuffer = cwBuffer;
 	DWORD dwMaxChars = _countof(cwBuffer);
 	DWORD dwLength = ::GetModuleFileNameA(importmodule, pszBuffer, dwMaxChars);
-#endif
+#endif
+
+    // have a stack local array of the addresses, don't want to use malloc for this
+    // The reason to precompute and cache these is because VirtualProtect is expensive
+    // Thus first we compute *only* once the real addresses for the export module
+    LPVOID realAddresses[MAX_PATCH_ENTRY_COUNT];
+    patchentry_t* patchEntry = patchModule->patchTable;
+    int i = 0;
+    while (patchEntry->importName)
+    {
+        LPCSTR importname = patchEntry->importName;
+
+        // Get the *real* address of the import. If we find this address in the IAT,
+        // then we've found the entry that needs to be patched.
+        LPVOID import = VisualLeakDetector::_RGetProcAddress(exportmodule, importname);
+        if (!import)
+            import = GetProcAddress(exportmodule, importname);
+        import = FindRealCode(import);
+
+        realAddresses[i] = import;
+
+        patchEntry++;
+        i++;
+
+        if (i >= MAX_PATCH_ENTRY_COUNT)
+        {
+            // we only process MAX_PATCH_ENTRY_COUNT, if we exceed it crash
+            // if this abort is ever hit, it means that MAX_PATCH_ENTRY_COUNT should be bumped up
+            Report(L"MAX_PATCH_ENTRY_COUNT is set to %zu, but has been exceeded, MAX_PATCH_ENTRY_COUNT needs to be increased.\n", MAX_PATCH_ENTRY_COUNT);
+            abort();
+            break;
+        }
+    }
 
     int result = 0;
     while (idte->FirstThunk != 0x0) {
         PCHAR importdllname = (PCHAR)R2VA(importmodule, idte->Name);
         UNREFERENCED_PARAMETER(importdllname);
 
-        patchentry_t *patchEntry = patchModule->patchTable;
-        int i = 0;
-        while(patchEntry->importName)
-        {
-            LPCSTR importname   = patchEntry->importName;
-            LPCVOID replacement = patchEntry->replacement;
-
-            // Get the *real* address of the import. If we find this address in the IAT,
-            // then we've found the entry that needs to be patched.
-            LPVOID import = VisualLeakDetector::_RGetProcAddress(exportmodule, importname);
-            if ( !import)
-                import = GetProcAddress(exportmodule, importname);
-            import = FindRealCode(import);
-
-            if (import == NULL) // Perhaps the named export module does not actually export the named import?
-            {
-                patchEntry++; i++;
-                continue;
-            }
-
-            // Locate the import's IAT entry.
-            IMAGE_THUNK_DATA *thunk = (IMAGE_THUNK_DATA*)R2VA(importmodule, idte->FirstThunk);
-            IMAGE_THUNK_DATA *origThunk = (IMAGE_THUNK_DATA*)R2VA(importmodule, idte->OriginalFirstThunk);
-            for (; origThunk->u1.Function != NULL;
-                origThunk++, thunk++)
-            {
-                LPVOID func = FindRealCode((LPVOID)thunk->u1.Function);
-                if (((DWORD_PTR)func == (DWORD_PTR)import))
+        // Locate the import's IAT entry.
+        IMAGE_THUNK_DATA *thunk = (IMAGE_THUNK_DATA*)R2VA(importmodule, idte->FirstThunk);
+        IMAGE_THUNK_DATA *origThunk = (IMAGE_THUNK_DATA*)R2VA(importmodule, idte->OriginalFirstThunk);
+        for (; origThunk->u1.Function != NULL;
+            origThunk++, thunk++)
+        {
+            LPVOID func = FindRealCode((LPVOID)thunk->u1.Function);
+
+            patchEntry = patchModule->patchTable;
+            i = 0;
+            while (patchEntry->importName)
+            {
+                LPVOID import = realAddresses[i];
+                if (import != NULL)
                 {
-                    // Found the IAT entry. Overwrite the address stored in the IAT
-                    // entry with the address of the replacement. Note that the IAT
-                    // entry may be write-protected, so we must first ensure that it is
-                    // writable.
-                    if (import != replacement)
+                    LPCVOID replacement = patchEntry->replacement;
+
+                    if (((DWORD_PTR)func == (DWORD_PTR)import))
                     {
-                        if (patchEntry->original != NULL)
-                            *patchEntry->original = func;
+                        // Found the IAT entry. Overwrite the address stored in the IAT
+                        // entry with the address of the replacement. Note that the IAT
+                        // entry may be write-protected, so we must first ensure that it is
+                        // writable.
+                        if (import != replacement)
+                        {
+                            if (patchEntry->original != NULL)
+                                *patchEntry->original = func;
 
-                        DWORD protect;
-                        if (VirtualProtect(&thunk->u1.Function, sizeof(thunk->u1.Function), PAGE_EXECUTE_READWRITE, &protect)) {
-                            thunk->u1.Function = (DWORD_PTR)replacement;
-                            if (VirtualProtect(&thunk->u1.Function, sizeof(thunk->u1.Function), protect, &protect)) {
+                            DWORD protect;
+                            if (VirtualProtect(&thunk->u1.Function, sizeof(thunk->u1.Function), PAGE_EXECUTE_READWRITE, &protect)) {
+                                thunk->u1.Function = (DWORD_PTR)replacement;
+                                if (VirtualProtect(&thunk->u1.Function, sizeof(thunk->u1.Function), protect, &protect)) {
 #ifdef PRINTHOOKINFO
-                                if (!IS_ORDINAL(importname)) {
-                                    DbgReport(L"Hook dll \"%S\" import %S!%S()\n",
-                                        strrchr(pszBuffer, '\\') + 1, patchModule->exportModuleName, importname);
-                                } else {
-                                    DbgReport(L"Hook dll \"%S\" import %S!%zu()\n",
-                                        strrchr(pszBuffer, '\\') + 1, patchModule->exportModuleName, importname);
-                                }
+                                    if (!IS_ORDINAL(importname)) {
+                                        DbgReport(L"Hook dll \"%S\" import %S!%S()\n",
+                                            strrchr(pszBuffer, '\\') + 1, patchModule->exportModuleName, importname);
+                                    }
+                                    else {
+                                        DbgReport(L"Hook dll \"%S\" import %S!%zu()\n",
+                                            strrchr(pszBuffer, '\\') + 1, patchModule->exportModuleName, importname);
+                                    }
 #endif
+                                }
                             }
                         }
+                        // The patch has been installed in the import module.
+                        result++;
+                        break;
                     }
-                    // The patch has been installed in the import module.
-                    result++;
-                    break;
-                }
 #ifdef PRINTHOOKINFO
-                PIMAGE_IMPORT_BY_NAME funcEntry = (PIMAGE_IMPORT_BY_NAME)
-                    R2VA(importmodule, origThunk->u1.AddressOfData);
-                if (stricmp(importdllname, patchModule->exportModuleName) == 0)
-                {
-                    if (!(origThunk->u1.Ordinal & IMAGE_ORDINAL_FLAG) && !IS_ORDINAL(importname) &&
-                        strcmp(reinterpret_cast<const char*>(funcEntry->Name), importname) == 0)
+                    PIMAGE_IMPORT_BY_NAME funcEntry = (PIMAGE_IMPORT_BY_NAME)
+                        R2VA(importmodule, origThunk->u1.AddressOfData);
+                    if (stricmp(importdllname, patchModule->exportModuleName) == 0)
                     {
-                        if (!dllNamePrinted)
+                        if (!(origThunk->u1.Ordinal & IMAGE_ORDINAL_FLAG) && !IS_ORDINAL(importname) &&
+                            strcmp(reinterpret_cast<const char*>(funcEntry->Name), importname) == 0)
                         {
-                            dllNamePrinted = true;
-                            DbgReport(L"Hook dll \"%S\":\n",
-                                strrchr(pszBuffer, '\\') + 1);
+                            if (!dllNamePrinted)
+                            {
+                                dllNamePrinted = true;
+                                DbgReport(L"Hook dll \"%S\":\n",
+                                    strrchr(pszBuffer, '\\') + 1);
+                            }
+                            DbgReport(L"Import found %S(\"%S\") for dll \"%S\".\n",
+                                importname, patchModule->exportModuleName, importdllname);
+                            break;
                         }
-                        DbgReport(L"Import found %S(\"%S\") for dll \"%S\".\n",
-                            importname, patchModule->exportModuleName, importdllname);
-                        break;
-                    }
-                    if ((origThunk->u1.Ordinal & IMAGE_ORDINAL_FLAG) && IS_ORDINAL(importname) &&
-                        (IMAGE_ORDINAL(origThunk->u1.Ordinal) == (UINT_PTR)importname))
-                    {
-                        if (!dllNamePrinted)
+                        if ((origThunk->u1.Ordinal & IMAGE_ORDINAL_FLAG) && IS_ORDINAL(importname) &&
+                            (IMAGE_ORDINAL(origThunk->u1.Ordinal) == (UINT_PTR)importname))
                         {
-                            dllNamePrinted = true;
-                            DbgReport(L"Hook dll \"%S\":\n",
-                                strrchr(pszBuffer, '\\') + 1);
+                            if (!dllNamePrinted)
+                            {
+                                dllNamePrinted = true;
+                                DbgReport(L"Hook dll \"%S\":\n",
+                                    strrchr(pszBuffer, '\\') + 1);
+                            }
+                            DbgReport(L"Import found %zu(\"%S\") for dll \"%S\".\n",
+                                importname, patchModule->exportModuleName, importdllname);
+                            break;
                         }
-                        DbgReport(L"Import found %zu(\"%S\") for dll \"%S\".\n",
-                            importname, patchModule->exportModuleName, importdllname);
-                        break;
                     }
-                }
-#endif
+#endif
+                }
+
+                patchEntry++; i++;
             }
-            patchEntry++; i++;
         }
 
         idte++;