Deadlock in chrome

c++ / delphi package - dll injection and api hooking
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Deadlock in chrome

Post by EaSy »

Hello,
we encountered an issue with chrome. It happened to us twice, unfortunately we cannot reproduce it anymore. The issue was deadlock in chrome. I have a dump with all the data from MCH.

The callstack of deadlocked thread is:

Code: Select all

0:043> kp
ChildEBP RetAddr  
1647c2ac 74bd3bd5 ntdll!ZwDelayExecution+0x15
1647c314 74bd44a5 KERNELBASE!SleepEx+0x65
1647c324 52be826a KERNELBASE!Sleep+0xf
1647c8cc 52be841f STGuard32!CCodeHook::~CCodeHook(void)+0x5fa [c:\prace\svn\tag\5.5.0.11\common\libraries\madcodehook\sources\c++\ccodehook.cpp @ 1055]
1647c8d8 52bdfe04 STGuard32!CCodeHook::`scalar deleting destructor'(void)+0xf
1647c934 52be043c STGuard32!UnhookInternal(void ** pNextHook = 0x52c95cc0, int dontUnhookHelperHooks = 0n1, int wait = 0n1)+0x1a4 [c:\prace\svn\tag\5.5.0.11\common\libraries\madcodehook\sources\c++\hooking.cpp @ 439]
1647c9e4 52be0f4c STGuard32!CheckHooks(struct HINSTANCE__ * hModule = 0x0d7a0000)+0x2bc [c:\prace\svn\tag\5.5.0.11\common\libraries\madcodehook\sources\c++\hooking.cpp @ 605]
1647ca20 71aa003b STGuard32!LdrLoadDllCallbackProc(unsigned long path = 0x165c81dc, unsigned long * flags = 0x1647ca64, struct tagUnicodeString * name = 0x1647ca4c, void ** handle = 0x1647ca58)+0xac [c:\prace\svn\tag\5.5.0.11\common\libraries\madcodehook\sources\c++\hooking.cpp @ 696]
WARNING: Frame IP not in any known module. Following frames may be wrong.
1647ca5c 6e365cff 0x71aa003b
1647ca7c 74a89d43 nvd3d9wrap!GetNVDisplayW+0x462e
1647ca98 74a89cc7 ole32!LoadLibraryWithLogging(LoadOrFreeWhy why = LoadingInprocServer (0n0), wchar_t * pwszFileName = 0x1647cb14 "C:\Program Files (x86)\Common Files\microsoft shared\ink\tiptsf.dll", unsigned long dwFlags = 8, struct HINSTANCE__ ** phMod = 0x1647cae8)+0x16 [d:\w7rtm\com\ole32\common\loadfree.cxx @ 157]
1647cabc 74a89bb6 ole32!CClassCache::CDllPathEntry::LoadDll(struct DLL_INSTANTIATION_PROPERTIES * dip = 0x1647cb14, <function> ** pfnGetClassObject = 0x1647cae0, <function> ** pfnDllCanUnload = 0x1647cae4, struct HINSTANCE__ ** hDll = 0x1647cae8)+0xa9 [d:\w7rtm\com\ole32\com\objact\dllcache.cxx @ 1925]
1647caec 74a890be ole32!CClassCache::CDllPathEntry::Create_rl(struct DLL_INSTANTIATION_PROPERTIES * dip = 0x1647cb14, struct ACTIVATION_PROPERTIES * ap = 0x1647cdfc, class CClassCache::CDllPathEntry ** pDPE = 0x1647cb0c)+0x37 [d:\w7rtm\com\ole32\com\objact\dllcache.cxx @ 1783]
1647cd38 74a88f93 ole32!CClassCache::CClassEntry::CreateDllClassEntry_rl(unsigned long dwContext = 1, struct ACTIVATION_PROPERTIES * ap = 0x1647cdfc, class CClassCache::CDllClassEntry ** pDCE = 0x1647cd68)+0xd4 [d:\w7rtm\com\ole32\com\objact\dllcache.cxx @ 886]
1647cd80 74a88e99 ole32!CClassCache::GetClassObjectActivator(unsigned long dwContext = 1, struct ACTIVATION_PROPERTIES * ap = 0x16582d5c, class CClassCache::IMiniMoniker ** ppIM = 0x1647cdac)+0x224 [d:\w7rtm\com\ole32\com\objact\dllcache.cxx @ 4795]
1647cdb8 74a88c57 ole32!CClassCache::GetClassObject(struct ACTIVATION_PROPERTIES * ap = 0x1647cdfc)+0x30 [d:\w7rtm\com\ole32\com\objact\dllcache.cxx @ 4574]
1647ce34 74aa3170 ole32!CServerContextActivator::CreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c)+0x110 [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 974]
1647ce74 74a88dca ole32!ActivationPropertiesIn::DelegateCreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesOut ** ppActPropsOut = 0x1647d96c)+0x108 [d:\w7rtm\com\ole32\actprops\actprops.cxx @ 1917]
1647cec8 74a88d3f ole32!CApartmentActivator::CreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c)+0x112 [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 2268]
1647cee8 74a88ac2 ole32!CProcessActivator::CCICallback(unsigned long dwContext = 1, struct IUnknown * pUnkOuter = 0x00000000, class ActivationPropertiesIn * pActIn = 0x1647d260, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c)+0x6d [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 1737]
1647cf08 74a88a73 ole32!CProcessActivator::AttemptActivation(class ActivationPropertiesIn * pActIn = 0x1647d260, struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c, <function> * pfnCtxActCallback = 0x74a8a94d, unsigned long dwContext = 1)+0x2c [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 1630]
1647cf44 74a88e2d ole32!CProcessActivator::ActivateByContext(class ActivationPropertiesIn * pActIn = 0x1647d260, struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c, <function> * pfnCtxActCallback = 0x74a8a94d)+0x4f [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 1487]
1647cf6c 74aa3170 ole32!CProcessActivator::CreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c)+0x49 [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 1377]
1647cfac 74aa2ef4 ole32!ActivationPropertiesIn::DelegateCreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesOut ** ppActPropsOut = 0x1647d96c)+0x108 [d:\w7rtm\com\ole32\actprops\actprops.cxx @ 1917]
1647d20c 74aa3170 ole32!CClientContextActivator::CreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesIn * pInActProperties = 0x1647d404, struct IActivationPropertiesOut ** ppOutActProperties = 0x1647d96c)+0xb0 [d:\w7rtm\com\ole32\com\objact\actvator.cxx @ 685]
1647d24c 74aa3098 ole32!ActivationPropertiesIn::DelegateCreateInstance(struct IUnknown * pUnkOuter = 0x00000000, struct IActivationPropertiesOut ** ppActPropsOut = 0x1647d96c)+0x108 [d:\w7rtm\com\ole32\actprops\actprops.cxx @ 1917]
1647da20 74aa9e25 ole32!ICoCreateInstanceEx(struct _GUID * Clsid = 0x1647db50 {807c1e6c-1d00-453f-b920-b61bb7cdd997}, struct IUnknown * punkOuter = 0x00000000, unsigned long dwClsCtx = 1, struct _COSERVERINFO * pServerInfo = 0x00000000, unsigned long dwCount = 1, unsigned long dwActvFlags = 0, struct tagMULTI_QI * pResults = 0x1647dac8, class ActivationPropertiesIn * pActIn = 0x00000000)+0x404 [d:\w7rtm\com\ole32\com\objact\objact.cxx @ 1334]
1647da80 74aa9d86 ole32!CComActivator::DoCreateInstance(struct _GUID * Clsid = 0x1647db50 {807c1e6c-1d00-453f-b920-b61bb7cdd997}, struct IUnknown * punkOuter = 0x00000000, unsigned long dwClsCtx = 1, struct _COSERVERINFO * pServerInfo = 0x00000000, unsigned long dwCount = 1, struct tagMULTI_QI * pResults = 0x1647dac8, class ActivationPropertiesIn * pActIn = 0x00000000)+0xd9 [d:\w7rtm\com\ole32\com\objact\immact.hxx @ 343]
1647daa4 74aa9d3f ole32!CoCreateInstanceEx(struct _GUID * Clsid = 0x1647db50 {807c1e6c-1d00-453f-b920-b61bb7cdd997}, struct IUnknown * punkOuter = 0x00000000, unsigned long dwClsCtx = 1, struct _COSERVERINFO * pServerInfo = 0x00000000, unsigned long dwCount = 1, struct tagMULTI_QI * pResults = 0x1647dac8)+0x38 [d:\w7rtm\com\ole32\com\objact\actapi.cxx @ 157]
1647dad4 6e36515d ole32!CoCreateInstance(struct _GUID * rclsid = 0x1647db50 {807c1e6c-1d00-453f-b920-b61bb7cdd997}, struct IUnknown * pUnkOuter = 0x00000000, unsigned long dwContext = 1, struct _GUID * riid = 0x75955084 {5e078e03-8265-4bbe-9487-d242edbef910}, void ** ppv = 0x165c81bc)+0x37 [d:\w7rtm\com\ole32\com\objact\actapi.cxx @ 110]
1647dafc 52b1aaee nvd3d9wrap!GetNVDisplayW+0x3a8c
1647db1c 71af003b STGuard32!CoCreateInstanceCallback(struct _GUID * rclsid = 0x1647db50 {807c1e6c-1d00-453f-b920-b61bb7cdd997}, struct IUnknown * pUnkOuter = 0x00000000, unsigned long dwClsContext = 1, struct _GUID * riid = 0x75955084 {5e078e03-8265-4bbe-9487-d242edbef910}, void ** ppv = 0x165c81bc)+0x1e [s:\5.6.0\discryptor\client service\modules\hooking\hooker.cpp @ 42]
1647dbb4 75954f60 0x71af003b
1647dc18 75955159 shell32!CAutoComplete::_CommonInit+0x1c3
1647dc3c 76a873cd shell32!CAutoComplete::Init+0x2c
1647dc70 76a8a063 comdlg32!AutoComplete+0xbd
1647dc8c 76a8b4a6 comdlg32!CFileOpenSave::ApplyAutoComplete+0x2a
1647dca8 76a8b433 comdlg32!CComboBoxExBase::_SetUpAutoComplete+0x41
1647dcc0 76a91c75 comdlg32!CFileNameComboBox::InitializeControl+0x13d
1647dcd8 76a8d70e comdlg32!CComboBoxExBase::Init+0x19
1647dd00 76a8d607 comdlg32!CFileOpenSave::_AddStandardControlsForLayout+0x101
1647dd0c 76a8845a comdlg32!CFileOpenSave::_PrepareControlsForLayout+0x29
1647df9c 76a87f97 comdlg32!CFileOpenSave::_InitOpenSaveDialog+0x4db
1647e208 74ef62fa comdlg32!CFileOpenSave::s_OpenSaveDlgProc+0x10b
1647e234 74f1f9df user32!InternalCallWinProc+0x23
1647e2b0 74f1f784 user32!UserCallDlgProcCheckWow+0xd7
1647e300 74f1f889 user32!DefDlgProcWorker+0xb7
1647e320 74ef62fa user32!DefDlgProcW+0x29
1647e34c 74ef6d3a user32!InternalCallWinProc+0x23
1647e3c4 74ef965e user32!UserCallWinProcCheckWow+0x109
1647e408 74f2206f user32!SendMessageWorker+0x581
1647e4dc 74f1cf4b user32!InternalCreateDialog+0xb9f
1647e514 74f1ce8a user32!InternalDialogBox+0xc1
1647e534 74f1cc0e user32!DialogBoxIndirectParamAorW+0x37
1647e554 76a8597b user32!DialogBoxIndirectParamW+0x1b
1647e5a0 76ac1b50 comdlg32!CFileOpenSave::Show+0x181
1647e5cc 76ac29d0 comdlg32!_InvokeNewFileOpenSave+0xab
1647e5f8 76ac2b15 comdlg32!_CreateNewFileOpenSaveInProc+0xae
1647e634 76ac2b71 comdlg32!NewGetFileName+0x121
1647e644 76ab9a9b comdlg32!NewGetOpenFileName+0xf
1647e670 76aba33f comdlg32!GetFileName+0xcd
1647f6f0 52b3d71a comdlg32!GetOpenFileNameW+0x6a
1647f750 70f3003b STGuard32!GetOpenFileNameWHook(struct tagOFNW * lpofn = 0x1647f7a8)+0x13a [s:\5.6.0\discryptor\client service\modules\injectiondll\getopenfilenamew.cpp @ 48]
1647f75c 03ccebba 0x70f3003b
1647f770 03ccedab chrome_2bb0000!RelaunchChromeBrowserWithNewCommandLineIfNeeded+0xcb0ec0
1647f784 03713034 chrome_2bb0000!RelaunchChromeBrowserWithNewCommandLineIfNeeded+0xcb10b1
1648f864 03712545 chrome_2bb0000!RelaunchChromeBrowserWithNewCommandLineIfNeeded+0x6f533a
1648f8fc 02c3834f chrome_2bb0000!RelaunchChromeBrowserWithNewCommandLineIfNeeded+0x6f484b
1648f90c 02c0f926 chrome_2bb0000!ChromeMain+0x7cce9
1648f9bc 02c0f4dd chrome_2bb0000!ChromeMain+0x542c0
1648f9f4 02c0ef87 chrome_2bb0000!ChromeMain+0x53e77
1648fb38 02c973fc chrome_2bb0000!ChromeMain+0x53921
1648fb6c 02c0eb66 chrome_2bb0000!ChromeMain+0xdbd96
1648fb8c 02c0ea78 chrome_2bb0000!ChromeMain+0x53500
1648fbb0 02c0e980 chrome_2bb0000!ChromeMain+0x53412
1648fbd8 02c0e8ea chrome_2bb0000!ChromeMain+0x5331a
1648fbfc 02c0aa7d chrome_2bb0000!ChromeMain+0x53284
1648fc28 02c0a660 chrome_2bb0000!ChromeMain+0x4f417
1648fc44 7500338a chrome_2bb0000!ChromeMain+0x4effa
1648fc50 76f39f72 kernel32!BaseThreadInitThunk+0xe
1648fc90 76f39f45 ntdll!__RtlUserThreadStart+0x70
1648fca8 00000000 ntdll!_RtlUserThreadStart+0x1b
We have GetOpenFileNameWHook, STGuard32!CoCreateInstanceCallback and MCH internal LdrLoadDllCallbackProc in play in here.
The CheckHooks is called for lib at baseaddress 0d7a0000:

Code: Select all

0:043> lmv a 0d7a0000
start    end        module name
0d7a0000 0d7f8000   tiptsf     (deferred)             
    Image path: C:\Program Files (x86)\Common Files\microsoft shared\ink\tiptsf.dll
    Image name: tiptsf.dll
    Timestamp:        Wed Jun 18 03:50:46 2014 (53A0F076)
    CheckSum:         00060D4E
    ImageSize:        00058000
    File version:     6.1.7601.18512
    Product version:  6.1.7601.18512
    File flags:       0 (Mask 3F)
    File OS:          40004 NT Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0409.04b0
    CompanyName:      Microsoft Corporation
    ProductName:      Microsoft® Windows® Operating System
    InternalName:     TipTsf.dll
    OriginalFilename: TipTsf.dll
    ProductVersion:   6.1.7601.18512
    FileVersion:      6.1.7601.18512 (win7sp1_gdr.140617-1517)
    FileDescription:  Tablet PC Input Panel Text Services Framework
    LegalCopyright:   © Microsoft Corporation. All rights reserved.
The UnhookInternal is called for pNextHook:

Code: Select all

0:043> dp 0x52c95cc0
52c95cc0  76aba2d5 52b3d7a0 70f60000 00000000
52c95cd0  52c25324 52c25314 52c25304 52c252f4
52c95ce0  52c252e4 52c252d0 52c252c4 52c252b0
52c95cf0  52c252a0 52c25294 52c25284 52c25274
52c95d00  52c25264 52c25258 52c2524c 52c25240
52c95d10  52c25230 52c25224 52c25210 52c25200
52c95d20  52c251f0 52c251e0 52c251d0 52c251c0
52c95d30  52c251ac 52c2519c 52c2518c 52c2517c
0:043> ln 76aba2d5
(76aba2d5)   comdlg32!GetOpenFileNameW   |  (76aba353)   comdlg32!GetSaveFileNameA
Exact matches:
    comdlg32!GetOpenFileNameW (<no parameter info>)

And this is wrong... naturally, you cannot unhook comdlg32!GetOpenFileNameW when it is in callstack.

For you i made dump of some MCH variables, so you can debug your code:

content of ach[58]

Code: Select all

0:043> .frame 6
06 1647c9e4 52be0f4c STGuard32!CheckHooks+0x2bc [c:\prace\svn\tag\5.5.0.11\common\libraries\madcodehook\sources\c++\hooking.cpp @ 605]
0:043> dv
        hModule = 0x76a80000
              p = 0x76aba2d5
              i = 0n58
             b1 = true
            ach = class CCollection<tagHookItem,CStructureEqualHelper<tagHookItem> >
      lastError = 0
        hModule = 0x0d7a0000
0:043> dt ach
Local var @ 0x1647c9b8 Type CCollection<tagHookItem,CStructureEqualHelper<tagHookItem> >
   +0x000 mpArray          : 0x00eadde0 tagHookItem
   +0x004 mCount           : 0n61
   +0x008 mSize            : 0n64
   +0x00c mSizeFixed       : 0
0:043> dt tagHookItem 0x00eadde0+(0n58*0x224) -r
STGuard32!tagHookItem
   +0x000 hOwner           : 0x52af0000 HINSTANCE__
      +0x000 unused           : 0n9460301
   +0x004 hModule          : (null) 
   +0x008 ModuleName       : [260]  "comdlg32.dll"
   +0x10c ProcName         : [260]  "GetOpenFileNameW"
   +0x210 pCode            : (null) 
   +0x214 pCodeHook        : (null) 
   +0x218 pNextHook        : 0x52c95cc0  -> 0x76aba2d5 Void
   +0x21c pCallback        : 0x52b3d5e0 Void
   +0x220 Flags            : 0
deleted CCodeHook:

Code: Select all

0:043> dt CCodeHook 00e63b90  -r
STGuard32!CCodeHook
   +0x000 __VFN_table : 0x52c1e7b8 
   +0x004 mpInUseArray     : 0x70f3003b tagInUse
      +0x000 push             : 0x68 'h'
      +0x001 returnAddress    : 0x03cce96b Void
      +0x005 lock             : 0xf0 ''
      +0x006 and              : 0x2583
      +0x008 andAddress       : 0x70f3003c Void
      +0x00c zero             : 0 ''
      +0x00d ret              : 0xc3 ''
   +0x008 mLeakUnhook      : 0
   +0x00c mPatchAddr       : 0x76aba2d5 tagAbsoluteJmp
      +0x000 Opcode           : 0x8b ''
      +0x001 modRm            : 0xff ''
      +0x002 Target           : 0xb8ec8b55
   +0x010 mNewCode         : tagAbsoluteJmp
      +0x000 Opcode           : 0xff ''
      +0x001 modRm            : 0x25 'Unknown format characterUnknown format control character
      +0x002 Target           : 0x70f2001e
   +0x016 mOldCode         : tagAbsoluteJmp
      +0x000 Opcode           : 0x8b ''
      +0x001 modRm            : 0xff ''
      +0x002 Target           : 0xb8ec8b55
   +0x01c mpHookQueue      : 0x00df20c0 CHookQueue
      +0x000 mMapName         : SString
         +0x000 mpData           : 0x0fc55db8  "NamedBuffer, mAH, Process $000014b0, API $76aba2d5"
         +0x004 mpEndPos         : 0x0fc55e1c  ""
         +0x008 mBufferLength    : 0x4c
         +0x00c mAllocLength     : 0n25
         +0x010 mDeleteBufferOnDestruct : 1
      +0x014 mHMap            : 0x00001fc0 Void
      +0x018 mpMapBuffer      : 0x0d570000  -> (null) 
      +0x01c mpBuffer         : 0x70f10000 Void
      +0x020 mpHookQueueHeader : 0x70f10000 tagHookQueueHeader
         +0x000 ItemCount        : 0n2
         +0x004 Capacity         : 0n8
         +0x008 hMap             : 0x00001fc0 Void
         +0x00c pPatchAddress    : 0x76aba2d5 tagAbsoluteJmp
         +0x010 OldCode          : tagAbsoluteJmp
         +0x016 NewCode          : tagAbsoluteJmp
      +0x024 mpHookRecords    : 0x70f1001c tagHookQueueRecord
         +0x000 pHookProc        : (null) 
         +0x004 ppNextHook       : 0x70f2001e  -> 0x70f20000 Void
   +0x020 mHModule         : 0x76a80000 HINSTANCE__
      +0x000 unused           : 0n9460301
   +0x024 mSafeHooking     : 0n0
   +0x028 mNoImproveUnhook : 0n0
   +0x02c mValid           : 1
   +0x02d mNewHook         : 1
   +0x02e mDestroying      : 1
   +0x02f mIsWinsock2      : 0
   +0x030 mPatchExportTable : 0
   +0x034 mpHookedFunction : 0x76aba2d5 Void
   +0x038 mpCallbackFunction : 0x70f3000a Void
   +0x03c mpShareCode      : 0x00240065 Void
   +0x040 mpNextHook       : 0x52c95cc0  -> 0x76aba2d5 Void
   +0x044 mpTramp          : 0x70f20000 Void
   +0x048 mpHookStub       : 0x70f30000 tagIndirectAbsoluteJump
      +0x000 Opcode           : 0xff ''
      +0x001 modRm            : 0x25 'Unknown format characterUnknown format control character
      +0x002 Target           : 0x70f30006
      +0x006 AbsoluteAddress  : 0x70f20000 Void
Any idea what happened and how to avoid such condition?

PP
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

Hmmmm... This looks very complicated.

First of all let me start with an explanation of two separate madCodeHook features which are involved in this:

1) madCodeHook has a feature called "safe unhooking" and "safe uninjection". This feature works like this: In the moment when you uninject your hook dll, madCodeHook tries to *first* unhook all APIs, before unloading your hook dll. Only after all hooks were successfully uninstalled, FreeLibrary is called. This logic makes sure that all the dirty work is done outside of DllMain, to avoid deadlocks. For every hooked API, madCodeHook first uninstalls the hook, and then it checks whether the hook is still "in use". E.g. if GetOpenFileNameW is hooked, but one of the running threads is still "inside" of GetOpenFileNameW (meaning, GetOpenFileNameWHook is in the callstack of a thread), madCodeHook enters a Sleep() loop and waits until that is no longer the case. Only then the hook installation is considered "complete". This logic results in DLL uninjection sometimes being delayed, sometimes even stuck. But it's an important feature to make uninjection as stable as possible.

2) There's one more automatic feature: madCodeHook automatically checks whether a hooked dll got unloaded and then maybe a different version loaded, and automatically uninstalls and reinstalls it hooks accordingly.

Now what seems to have happened in your specific case is that "GetOpenFileNameW" was called, which you've hooked. This API internally seems to have called LoadLibrary("tiptsf.dll"). Whenever a new dll is loaded, madCodeHook's feature 2) is invoked which enumerates through all installed API hooks and checks every one of them, whether they have to be uninstalled and reinstalled. For some reason that is not clear to me it seems that in this specific situation madCodeHook thinks that the "GetOpenFileNameW" API has been changed to a different address in RAM (maybe because the underlying DLL got unloaded and reloaded to a different address), and as a result madCodeHook tries to uninstall and reinstall the GetOpenFileNameW hook. Usually that works fine. However, in this case feature 1) results in madCodeHook shooting itself in the foot. The unhooking of GetOpenFileNameW is delayed, because madCodeHook thinks that the GetOpenFileNameW hook is still in use (correctly so). But unfortunately it's delayed indefinitely because it's the very same thread that is still in GetOpenFileNameW, so the Sleep loop runs indefinitely.

Ok, I hope these explanations have helped you understand the cause and exact circumstances of the problem.

For me the big question is: Why does madCodeHook think that the GetOpenFileNameW hook needs to be reinstalled? It's rather unlikely that that is really the case. So I think something in madCodeHook's "CheckHooks()" function (which is responsible for feature 2)) isn't working correctly. Since we're talking about Chrome, it's possible that this problem is caused by the Chrome sandbox blocking "something" which leads madCodeHook to come to a wrong conclusion.

It's very unfortunate that you can't seem to reproduce this issue, anymore. This is going to be very hard to fix without being able to reproduce it. Can't you reproduce it, anymore, *AT ALL*? Or just very rarely now?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

Thx for explanations.

it looks like the hooks for GetOpenFileName and GetSaveFileName are both in hook collection twice at indexes 57, 58, 59 and 60.

Code: Select all

0:043> dt tagHookItem 0x00eadde0+(0n57*0x224)
STGuard32!tagHookItem
   +0x000 hOwner           : 0x52af0000 HINSTANCE__
   +0x004 hModule          : (null) 
   +0x008 ModuleName       : [260]  "comdlg32.dll"
   +0x10c ProcName         : [260]  "GetSaveFileNameW"
   +0x210 pCode            : (null) 
   +0x214 pCodeHook        : (null) 
   +0x218 pNextHook        : 0x52c95cc8  -> 0x70f60000 Void
   +0x21c pCallback        : 0x52b3d7a0 Void
   +0x220 Flags            : 0
0:043> dt tagHookItem 0x00eadde0+(0n58*0x224)
STGuard32!tagHookItem
   +0x000 hOwner           : 0x52af0000 HINSTANCE__
   +0x004 hModule          : (null) 
   +0x008 ModuleName       : [260]  "comdlg32.dll"
   +0x10c ProcName         : [260]  "GetOpenFileNameW"
   +0x210 pCode            : (null) 
   +0x214 pCodeHook        : (null) 
   +0x218 pNextHook        : 0x52c95cc0  -> 0x76aba2d5 Void
   +0x21c pCallback        : 0x52b3d5e0 Void
   +0x220 Flags            : 0
0:043> dt tagHookItem 0x00eadde0+(0n59*0x224)
STGuard32!tagHookItem
   +0x000 hOwner           : 0x52af0000 HINSTANCE__
   +0x004 hModule          : 0x76a80000 HINSTANCE__
   +0x008 ModuleName       : [260]  "comdlg32.dll"
   +0x10c ProcName         : [260]  "GetSaveFileNameW"
   +0x210 pCode            : 0x76aba36e Void
   +0x214 pCodeHook        : 0x00e63a30 CCodeHook
   +0x218 pNextHook        : 0x52c95cc8  -> 0x70f60000 Void
   +0x21c pCallback        : 0x52b3d7a0 Void
   +0x220 Flags            : 0
0:043> dt tagHookItem 0x00eadde0+(0n60*0x224)
STGuard32!tagHookItem
   +0x000 hOwner           : 0x52af0000 HINSTANCE__
   +0x004 hModule          : 0x76a80000 HINSTANCE__
   +0x008 ModuleName       : [260]  "comdlg32.dll"
   +0x10c ProcName         : [260]  "GetOpenFileNameW"
   +0x210 pCode            : 0x76aba2d5 Void
   +0x214 pCodeHook        : 0x00e63b90 CCodeHook
   +0x218 pNextHook        : 0x52c95cc0  -> 0x76aba2d5 Void
   +0x21c pCallback        : 0x52b3d5e0 Void
   +0x220 Flags            : 0
This could be reason. But how is this possible. They have the same pNextHook in both cases.
I am sure that we didn't called HookApi or HookCode twice. Maybe some exception at the wrong time in the wrong place?
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

Hmmmm... In the moment when you call HookAPI(), if the to-be-hooked DLL is not loaded yet, there's an HookItem added with "pCode = NULL" and "pCodeHook = NULL". This HookItem is just a reminder for madCodeHook to automatically install the API hook later, when the to-be-hooked DLL is finally loaded. Then in CheckHooks(), this "pCode = NULL" item is supposed to be removed and replaced with a HookItem with properly set "pCode" and "pCodeHook" values. In your list of HookItems it appears that both the "pCode = NULL" and "pCode = something" variants are in the list. One reason for that could be that removing the "pCode = NULL" item didn't work. Another reason could be that it really was in there twice from the start. But neither makes much sense to me. If you look at the CheckHooks() code, it reads:

Code: Select all

            if ((!ach[i].pCodeHook) || (ach[i].pCode != p))
            {
              UnhookInternal(ach[i].pNextHook, true, true);
              HookCodeInternal(...);
And UnhookInternal() does:

Code: Select all

          g_pHookCollection->RemoveAt(i);
I'm not sure how that could not work, unless the whole collection code is broken. Would it possible/hard for you to check whether the HookItems are there twice after you're done with your HookAPI() calls? And then maybe you could manually load comdlg32.dll and then check again how that affected the HookItems?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

Maybe a race condition in CheckHooks?

Multiple UnhookInternal and HookCodeInternal concurently running at the same time in the heavily threaded environment like chrome... Maybe it could cause something like this.

What do you think?
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

I don't know. Well, I guess it's not impossible. I've protected the most important code sections with critical sections, but that's mostly to guarantee data integrity and avoid crashes. Things like CheckHooks() running in two separate threads at the same time can happen. But then, shouldn't we get two items with both "pCode" properly filled? CheckHooks() first uninstalls the old hook and removes the old HookItem, then adds the new one. So if two threads run at the same time, they could both try to remove the old HookItem. Only one would succeed. Then they would both add the new HookItem. But that would result in both HookItems having "pCode" properly set. At least that's as far as I can see. I don't really see how any of this could result in having two HookItems, one with "pCode = NULL" and one with "pCode" properly set. Thoughts?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

What if comdlg32 wasn't loaded while the concurency occured.

Code: Select all

Thread1
    Thread2
UnhookInternal(GetOpenFileName); (OK)
    UnhookInternal(GetOpenFileName); (FAIL)
HookCodeInternal(GetOpenFileName); (partial OK)
    HookCodeInternal(GetOpenFileName); (partial OK)
This would lead into this exact case: two empty GetOpenFileName tagHookItem structs in one buffer.
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

True!! :D

You can't reproduce this at all, anymore? I'll add a fix for this possible threading issue. It's unfortunate if we can't validify whether the fix actually works.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

We will try to check it in our testing facility.

You must have an application without comdlg32 at the beginning, then heavily call LoadLibrary and then try to load comdlg32. Can't say it is impossible, but it is rare...
We will see.

Thank you.
PP
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

We are not able to reproduce the issue with a good chance of reproducibility. It is quite rare in chrome and opera. I will try to write a test case since we now know when it happens.

Hook commdlg32
Call LoadLibrary in multiple threads
Load commdlg32
Call GetOpenFileName (force it somehow to call LoadLibrary inside)
Hope for dealock.
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

I'll create a new test build with some fixes for this soon. Hopefully even if it's only rarely reproducable for you, you might then be able to confirm (or not) whether the changes worked or not. Will let you know as soon as the test build is available.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

I am now able to reproduce it 100% with my testcase. I modified a source code of CheckHooks in this way:

Code: Select all

UnhookInternal(ach[i].pNextHook, true, true);
Sleep(250);
HookCodeInternal(
    ach[i].hOwner, 
    hModule,
    ach[i].ModuleName,
    p,
    ach[i].pCallback,
    ach[i].pNextHook,
    ach[i].Flags);
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

Very good, so we're probably on the right track...
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Deadlock in chrome

Post by madshi »

Ok, here's a new test build:

http://madshi.net/madCollectionBeta.exe (2.7.10.5)

This test build has two changes to fix this problem:

1) The "UnhookInternal + HookCodeInternal" is not executed at all for hook items, anymore, if the module of the to-be-hooked APIs is not even loaded yet. This should also save performance.
2) Once the "HookCodeInternal" is complete, it will double check the final "hook collection" to see if it's already listed there. If it is, it uninstalls itself again right away.

I'm not fully happy with 2) because there can be a race condition where a hook is installed twice and then one is uninstalled again. But the only other alternative would be to put the whole CheckHooks() function into a big critical section, which I find scary because I fear it could reduce performance too much.

At least the whole "hook collection" adding, deleting and double checking is already done in a critical section, so I think 2) should work correctly, even if it's not a pretty solution.

What do you think?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Deadlock in chrome

Post by EaSy »

Looks like it is working for me. I will msg you if anything goes wrong.

Thx,
PP
Post Reply