Skip to content

Commit 6469fc6

Browse files
committed
cygthread: suspend thread before terminating.
This addresses an extremely difficult to debug deadlock when running under emulation on ARM64. A relatively easy way to trigger this bug is to call `fork()`, then within the child process immediately call another `fork()` and then `exit()` the intermediate process. It would seem that there is a "code emulation" lock on the wait thread at this stage, and if the thread is terminated too early, that lock still exists albeit without a thread, and nothing moves forward. It seems that a `SuspendThread()` combined with a `GetThreadContext()` (to force the thread to _actually_ be suspended, for more details see https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743) makes sure the thread is "booted" from emulation before it is suspended. Hopefully this means it won't be holding any locks or otherwise leave emulation in a bad state when the thread is terminated. Also, attempt to use `CancelSynchonousIo()` (as seen in `flock.cc`) to avoid the need for `TerminateThread()` altogether. This doesn't always work, however, so was not a complete fix for the deadlock issue. Addresses: https://cygwin.com/pipermail/cygwin-developers/2024-May/012694.html Signed-off-by: Jeremy Drake <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
1 parent 6183b83 commit 6469fc6

File tree

2 files changed

+16
-1
lines changed

2 files changed

+16
-1
lines changed

winsup/cygwin/cygthread.cc

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,20 @@ cygthread::terminate_thread ()
302302
if (!inuse)
303303
goto force_notterminated;
304304

305+
if (_my_tls._ctinfo != this)
306+
{
307+
CONTEXT context;
308+
context.ContextFlags = CONTEXT_CONTROL;
309+
/* SuspendThread makes sure a thread is "booted" from emulation before
310+
it is suspended. As such, the emulator hopefully won't be in a bad
311+
state (aka, holding any locks) when the thread is terminated. */
312+
SuspendThread (h);
313+
/* We need to call GetThreadContext, even though we don't care about the
314+
context, because SuspendThread is asynchronous and GetThreadContext
315+
will make sure the thread is *really* suspended before returning */
316+
GetThreadContext (h, &context);
317+
}
318+
305319
TerminateThread (h, 0);
306320
WaitForSingleObject (h, INFINITE);
307321
CloseHandle (h);

winsup/cygwin/sigproc.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,8 @@ proc_terminate ()
410410
if (!have_execed || !have_execed_cygwin)
411411
chld_procs[i]->ppid = 1;
412412
if (chld_procs[i].wait_thread)
413-
chld_procs[i].wait_thread->terminate_thread ();
413+
if (!CancelSynchronousIo (chld_procs[i].wait_thread->thread_handle ()))
414+
chld_procs[i].wait_thread->terminate_thread ();
414415
/* Release memory associated with this process unless it is 'myself'.
415416
'myself' is only in the chld_procs table when we've execed. We
416417
reach here when the next process has finished initializing but we

0 commit comments

Comments
 (0)