On 1/28/20 2:09 PM, Jacek Caban wrote:
On 27.01.2020 23:19, Rémi Bernon wrote:
On 1/27/20 10:58 PM, Jacek Caban wrote:
Hi Rémi,
On 27/01/2020 13:07, Rémi Bernon wrote:
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
What's wrong about that? Suspending process on thread creation shouldn't be a problem. The debugger should process this event, which will resume the new thread and let it continue to the break point. Before introducing APC_BREAK_PROCESS I had to fix it in another place in winedbg, see commit 1bb98982d682a85b82e, maybe there is similar problem in gdb proxy?
Thanks,
Jacek
In gdbproxy mode, winedbg calls DebugBreakProcess whenever Ctrl-C is pressed, then WaitForDebugEvent for the corresponding event.
However *sometimes* DebugBreakProcess never returns and it cannot wait for the event and process it unless it is done in another thread -which would require changes in gdbproxy I would rather avoid. That may be how winedbg works in frontend mode?
The reason why DebugBreakProcess sometimes returns but not always is that it requests the APC, which completion notification is sometimes received by the server before the exception is raised and all threads are suspended, but sometimes it isn't. I don't think this was intended, and it would probably be better if DebugBreakProcess returns -or not- in a consistent way.
OK, I see, there is a problem. But I think that with your patch the race is still there, it's just much less likely. Server may have already sent the signal before client sent select request, but it may be blocked until server call is finishes. Then, if select returns system APC, it will be immediately interrupted by the signal.
Jacek
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:
1) 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.
2) 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.