Suspected deadlock in Tortoise 1.6.3 and greater

Hi everyone, I hope this is the best place to post this.

Lately we have been reports from users of our software (a file manager -
Directory Opus) that they are getting "lock ups" seemingly at random since
they upgraded to Tortoise 1.6.3 (and beyond). From our investigations it
would seem that the latest versions of Tortoise SVN have a flaw in the way
they use a critical section, which can easily manifest as a deadlock when
Tortoise is accessed from multiple threads simultaneously.

Consider the two following partial stack traces, from 2 different threads.
You can see they are both waiting to enter critical sections.

-------------------- THREAD 1 ---------------------
ntdll!ZwWaitForSingleObject+0xc
ntdll!RtlpWaitForCriticalSection+0x132
ntdll!RtlEnterCriticalSection+0x46
TortoiseSVN!ATL::CComCritSecLock<ATL::CComCriticalSection>::Lock+0x1b
[c:\program files\microsoft visual studio 9.0\vc\atlmfc\include\atlbase.h @
337]
TortoiseSVN!ShellCache::IsPathAllowed+0x47
[d:\development\svn\releases\tortoisesvn-1.6.6\src\tortoiseshell\shellcache.
h @ 321]
TortoiseSVN!CShellExt::IsMemberOf+0x20e
[d:\development\svn\releases\tortoisesvn-1.6.6\src\tortoiseshell\iconoverlay
.cpp @ 124]
WARNING: Stack unwind information not available. Following frames may be
wrong.
TortoiseOverlays!DllGetClassObject+0xb4c
shell32!CFSIconOverlayManager::_FindMatchingID+0x3b
shell32!CFSIconOverlayManager::GetFileOverlayInfo+0x3e
shell32!CFSFolder::_GetOverlayInfo+0x121
shell32!CFSFolder::GetOverlayIndex+0x31
shell32!_GetFileInfoSections+0x1ec
shell32!SHGetFileInfoW+0x13a
dopus!SHGetFileInfoSmart+0x46
<....>
-------------------- THREAD 1 ---------------------

-------------------- THREAD 2 ---------------------
ntdll!ZwWaitForSingleObject+0xc
ntdll!RtlpWaitForCriticalSection+0x132
ntdll!RtlEnterCriticalSection+0x46
shell32!LookupFileClass+0x15
<... a few lines deleted...>
shell32!CRegFolder::BindToObject+0x45
shell32!SHBindToObjectEx+0x3f
shell32!SHBindToFolderIDListParent+0x26
shell32!SHBindToIDListParent+0x18
shell32!SHGetPathFromIDListEx+0x54
shell32!SHGetPathFromIDListW+0x12
TortoiseSVN!ShellCache::ExcludeListValid+0x42a
[d:\development\svn\releases\tortoisesvn-1.6.6\src\tortoiseshell\shellcache.
h @ 573]
TortoiseSVN!ShellCache::IsPathAllowed+0x4ec
[d:\development\svn\releases\tortoisesvn-1.6.6\src\tortoiseshell\shellcache.
h @ 343]
TortoiseSVN!CShellExt::GetItemData+0x40
[d:\development\svn\releases\tortoisesvn-1.6.6\src\tortoiseshell\columnprovi
der.cpp @ 195]
dopus!SafeIColumnProviderGetItemData+0x51
<....>
-------------------- THREAD 2 ---------------------

Our supposition is that shell32.dll has an internal critical section that is
used by many shell functions.

Thread 1 is calling the SHGetFileInfo function. Now our supposition is that
as part of the internal workings of the SHGetFileInfo function, the shell
enters its critical section at this point. It then calls into Tortoise,
presumably via its IShellIconOverlayIdentifier interface (we couldn't find
the debug symbols for TortoiseOverlay.dll so can't be 100% sure of this),
and in ShellCache::IsPathAllowed, Tortoise attempts to enter its own
"m_critSec" critical section. This call blocks, because Thread 2 already has
that critical section held (see below).

Thread 2 is calling Tortoise directly (via its IColumnProvider interface).
In ShellCache::ExcluseListValid, Tortoise enters the critical section
"m_critSec", and then with that critical section still held, calls into the
shell via SHGetPathFromIDList. The shell, as part of its internal workings,
calls shell32!LookupFileClass, which attempts to enter the critical section
that is currently held by thread 1. Therefore we have a deadlock.

From what we can tell, the code in ShellCache::ExcludeListValid that calls
SHGetPathFromIDList was merged into the trunk as part of the 1.6.3 revision,
which fits exactly with our users' reports that the problem goes away if
they downgrade to 1.6.2.

I've attached a suggested solution. Basically the code that adds the three
default paths to exclude has been moved to the top of IsPathAllowed, and the
path strings are added to a new local vector. Crucially this takes place
BEFORE the critical section is entered. This vector is then passed to
ExcludeListValid which now simply copies the strings rather than querying
the shell directly.

It would be great if this problem could be fixed in the next update to
Tortoise, as we have more than a few users experiencing this issue.

Please let me know if you have any comments or questions about any of the
above.

Regards,
Jonathan Potter
GP Software

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=2426141

To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].