From 87e97ee305627f01ad2b17220fd9a7aa2c3bef30 Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Wed, 12 Aug 2015 18:19:16 -0700 Subject: [PATCH] adb: win32: fix shutdown deadlock adb can hang at shutdown due to a deadlock relating to WSACleanup(). This works around the issue by not calling WSACleanup() which shouldn't be done anyway since threads aren't done using Winsock at shutdown. A quick way to reproduce the original problem is to run many instances of adb, many of which will call exit() soon: for /l %i in (1,1,20) do @start adb nodaemon server You may have to boost the 20 to 200, or set ADB_TRACE=1 or use Windows 10 instead of Windows 7, to affect the timing, but eventually there should be hung adb processes with that repro. A more complete fix to prevent problems like this from occuring in the future, would be to additionally do the following: - Investigate all static destructors that are called when exit() is called. - If they don't do anything important, switch all calls to exit() to instead call _exit() and then ban exit() from being called. Change-Id: Id1be3bf0053809a45f2eca4461e4c35b5ef9388d Signed-off-by: Spencer Low --- adb/sysdeps_win32.cpp | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index db552a2da..f52571310 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -651,21 +651,11 @@ static int _fh_socket_write(FH f, const void* buf, int len) { static int _winsock_init; -static void -_cleanup_winsock( void ) -{ - // TODO: WSAStartup() might be called multiple times and this won't properly - // cleanup the right number of times. Plus, WSACleanup() probably doesn't - // make sense since it might interrupt other threads using Winsock (since - // our various threads are not explicitly cleanly shutdown at process exit). - WSACleanup(); -} - static void _init_winsock( void ) { // TODO: Multiple threads calling this may potentially cause multiple calls - // to WSAStartup() and multiple atexit() calls. + // to WSAStartup() which offers no real benefit. if (!_winsock_init) { WSADATA wsaData; int rc = WSAStartup( MAKEWORD(2,2), &wsaData); @@ -673,8 +663,21 @@ _init_winsock( void ) fatal( "adb: could not initialize Winsock: %s", SystemErrorCodeToString( rc ).c_str()); } - atexit( _cleanup_winsock ); _winsock_init = 1; + + // Note that we do not call atexit() to register WSACleanup to be called + // at normal process termination because: + // 1) When exit() is called, there are still threads actively using + // Winsock because we don't cleanly shutdown all threads, so it + // doesn't make sense to call WSACleanup() and may cause problems + // with those threads. + // 2) A deadlock can occur when exit() holds a C Runtime lock, then it + // calls WSACleanup() which tries to unload a DLL, which tries to + // grab the LoaderLock. This conflicts with the device_poll_thread + // which holds the LoaderLock because AdbWinApi.dll calls + // setupapi.dll which tries to load wintrust.dll which tries to load + // crypt32.dll which calls atexit() which tries to acquire the C + // Runtime lock that the other thread holds. } }