On 28.01.2020 14:43, Rémi Bernon wrote:
Let's name these locations:
[A]: @@ -1591,6 +1595,7 @@ DECL_HANDLER(select) if (apc->call.type != APC_NONE && (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) { + current->in_apc = 1; reply->call = apc->call; release_object( apc ); break;
[B]: @@ -1575,6 +1577,8 @@ DECL_HANDLER(select) wake_up( &apc->obj, 0 ); close_handle( current->process, req->prev_apc ); release_object( apc ); + current->in_apc = 0; + stop_thread_if_suspended( current ); }
reply->timeout = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout );
[C]: @@ -583,6 +584,7 @@ static void set_thread_info( struct thread *thread, /* stop a thread (at the Unix level) */ void stop_thread( struct thread *thread ) { + if (thread->in_apc) return; /* currently doing apc, will be suspended on return */ if (thread->context) return; /* already inside a debug event, no need for a signal */ /* can't stop a thread while initialisation is in progress */ if (is_process_init_done(thread->process)) send_thread_signal( thread, SIGUSR1 );
When the APC_BREAK_PROCESS executes, a servicing thread calls the select request, which returns through [A], flagging the thread.
It returns from the select request, and does the APC call -which creates the exception thread. Then, I see two possible ordering:
- The servicing thread calls first the select request to notify APC
completion, goes to [B] which wakes the APC caller -resuming DbgBreakProcess on the debugger side, and unflags the thread but it's not suspended yet. The exception thread does the queue_exception_event request, which ultimately goes to [C] when suspending the process. The servicing thread is not flagged, so it is suspended.
- The exception thread does the queue_exception_event first, which
goes to [C], but as the servicing thread is still flagged, do not suspend it. The servicing thread then does the select request, goes to [B] which wakes the APC caller -resuming DbgBreakProcess- then unflags and suspends it as it should be.
Now I agree there could still be some race conditions in case 2):
- The additional thread suspension in [B] is done after the wake_up
for the APC caller, it should probably be done before, just to be sure.
- If the debugger is calling WaitForDebugEvent in parallel to the
DbgBreakProcess call. In this case, the debugger could maybe be notified of a debug event after the exception thread created it but before the servicing thread notifies of the APC completion. I don't if that's supposed to be supported, or how bad it is to have the servicing thread still alive for a bit.
If we want to have the later scenario be race-free, it would require to link the debug even only when the servicing thread is suspended. Or, to synchronize the servicing and exception thread somehow so that the servicing always does its select request first.
I think the more general problem is that any system APCs may be interrupted and suspended. If DbgBreakProcess is the only concern, we could probably hack it to not wait for APC result at all - it doesn't really need to (except for propagating OOM-like errors). For the general problem, let me use an example with another thread that just happens to suspend the process about the same time as DbgBreakProcess happens:
1. Debuggee thread calls server_select and enters the uninterrupted section in wine_server_call, but does not yet send the request
2. Thread 1 (of any process) requests suspending of debuggee process
3. Debugger's thread 2 calls DbgBreakProcess on debuggee process and waits for APC completion
4. Server processes suspend request from (2) and sends signals to all debuggee threads
5. Server processes break APC request from (3) and queues an APC
6. Debuggee thread from (1) finally sends the request and server replies with queued APC
7. Debuggee thread leaves uninterrupted section, and receives queued signal, which suspends the thread while it's about to process a system APC
You could maybe detect the problem on server side in (6), return pending and let signal handler to handle the APC. I'm a bit sceptical about that solution.
Another solution would be to ensure that system APCs are ran with signals blocked on client side. It's quite tricky through as it would require disabling signals while waiting in server_select (except for user APCs). This could be fine if we passed CPU context in select request, so that server wouldn't need any signal to suspend the thread.
Jacek