Whenever alloc_handle fails, we ignored the error and dequeued the next APC. This patch makes the loop break whenever the error status changes.
Note that the APC is still marked as executed although it failed.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- server/thread.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/server/thread.c b/server/thread.c index aec4c1acf4e..0d502eecba5 100644 --- a/server/thread.c +++ b/server/thread.c @@ -1581,26 +1581,23 @@ DECL_HANDLER(select)
reply->timeout = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout );
- if (get_error() == STATUS_USER_APC) + while (get_error() == STATUS_USER_APC) { - for (;;) + if (!(apc = thread_dequeue_apc( current, !(req->flags & SELECT_ALERTABLE) ))) + break; + /* Optimization: ignore APC_NONE calls, they are only used to + * wake up a thread, but since we got here the thread woke up already. + */ + if (apc->call.type != APC_NONE && + (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) { - if (!(apc = thread_dequeue_apc( current, !(req->flags & SELECT_ALERTABLE) ))) - break; - /* Optimization: ignore APC_NONE calls, they are only used to - * wake up a thread, but since we got here the thread woke up already. - */ - if (apc->call.type != APC_NONE && - (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) - { - reply->call = apc->call; - release_object( apc ); - break; - } - apc->executed = 1; - wake_up( &apc->obj, 0 ); + reply->call = apc->call; release_object( apc ); + break; } + apc->executed = 1; + wake_up( &apc->obj, 0 ); + release_object( apc ); } }
Signed-off-by: Rémi Bernon rbernon@codeweavers.com ---
Notes: This uses the same error handling logic in case of alloc_handle failure as for the user APC loop, the APC is considered as executed although it failed.
dlls/ntdll/server.c | 2 +- dlls/ntdll/sync.c | 2 +- server/thread.c | 22 +++++++++++++++++----- 3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 089eb3b89aa..73f4d86cf4d 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -624,7 +624,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT } SERVER_END_REQ; if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); - if (ret != STATUS_USER_APC) break; + if (ret != STATUS_USER_APC && ret != STATUS_KERNEL_APC) break; if (invoke_apc( &call, &result )) { /* if we ran a user apc we have to check once more if additional apcs are queued, diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index c4885973b68..d09b90a9273 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -2497,7 +2497,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size RtlLeaveCriticalSection( &addr_section );
if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); - if (ret != STATUS_USER_APC) break; + if (ret != STATUS_USER_APC && ret != STATUS_KERNEL_APC) break; if (invoke_apc( &call, &result )) { /* if we ran a user apc we have to check once more if additional apcs are queued, diff --git a/server/thread.c b/server/thread.c index 0d502eecba5..886b478c9ed 100644 --- a/server/thread.c +++ b/server/thread.c @@ -749,7 +749,7 @@ static int check_wait( struct thread *thread ) assert( wait );
if ((wait->flags & SELECT_INTERRUPTIBLE) && !list_empty( &thread->system_apc )) - return STATUS_USER_APC; + return STATUS_KERNEL_APC;
/* Suspended threads may not acquire locks, but they can run system APCs */ if (thread->process->suspend + thread->suspend > 0) return -1; @@ -1083,12 +1083,11 @@ void thread_cancel_apc( struct thread *thread, struct object *owner, enum apc_ty }
/* remove the head apc from the queue; the returned object must be released by the caller */ -static struct thread_apc *thread_dequeue_apc( struct thread *thread, int system_only ) +static struct thread_apc *thread_dequeue_apc( struct thread *thread, int system ) { struct thread_apc *apc = NULL; - struct list *ptr = list_head( &thread->system_apc ); + struct list *ptr = list_head( system ? &thread->system_apc : &thread->user_apc );
- if (!ptr && !system_only) ptr = list_head( &thread->user_apc ); if (ptr) { apc = LIST_ENTRY( ptr, struct thread_apc, entry ); @@ -1583,7 +1582,7 @@ DECL_HANDLER(select)
while (get_error() == STATUS_USER_APC) { - if (!(apc = thread_dequeue_apc( current, !(req->flags & SELECT_ALERTABLE) ))) + if (!(apc = thread_dequeue_apc( current, 0 ))) break; /* Optimization: ignore APC_NONE calls, they are only used to * wake up a thread, but since we got here the thread woke up already. @@ -1599,6 +1598,19 @@ DECL_HANDLER(select) wake_up( &apc->obj, 0 ); release_object( apc ); } + + if (get_error() == STATUS_KERNEL_APC) + { + apc = thread_dequeue_apc( current, 1 ); + if ((reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) + reply->call = apc->call; + else + { + apc->executed = 1; + wake_up( &apc->obj, 0 ); + } + release_object( apc ); + } }
/* queue an APC for a thread or process */
Signed-off-by: Jacek Caban jacek@codeweavers.com
The wait_select_reply call may return STATUS_USER_APC/STATUS_KERNEL_APC, depending on which APC is about to be returned but the apc call will always be APC_NONE right after the wait. It needs an additional select request to actually return the call.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/ntdll/server.c | 17 ++++++++++------- dlls/ntdll/sync.c | 10 ++++++---- 2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 73f4d86cf4d..18ee41790f3 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -607,7 +607,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT
memset( &result, 0, sizeof(result) );
- for (;;) + do { SERVER_START_REQ( select ) { @@ -623,9 +623,13 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT call = reply->call; } SERVER_END_REQ; - if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); - if (ret != STATUS_USER_APC && ret != STATUS_KERNEL_APC) break; - if (invoke_apc( &call, &result )) + + /* don't signal multiple times */ + if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT) + size = offsetof( select_op_t, signal_and_wait.signal ); + + if ((ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC) && + invoke_apc( &call, &result )) { /* if we ran a user apc we have to check once more if additional apcs are queued, * but we don't want to wait */ @@ -634,10 +638,9 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT size = 0; }
- /* don't signal multiple times */ - if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT) - size = offsetof( select_op_t, signal_and_wait.signal ); + if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); } + while (ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC);
if (ret == STATUS_TIMEOUT && user_apc) ret = STATUS_USER_APC;
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index d09b90a9273..3b8b4f4eaaa 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -2470,7 +2470,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
memset( &result, 0, sizeof(result) );
- for (;;) + do { RtlEnterCriticalSection( &addr_section ); if (!compare_addr( addr, cmp, size )) @@ -2496,16 +2496,18 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
RtlLeaveCriticalSection( &addr_section );
- if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); - if (ret != STATUS_USER_APC && ret != STATUS_KERNEL_APC) break; - if (invoke_apc( &call, &result )) + if ((ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC) && + invoke_apc( &call, &result )) { /* if we ran a user apc we have to check once more if additional apcs are queued, * but we don't want to wait */ abs_timeout = 0; user_apc = TRUE; } + + if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie ); } + while (ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC);
if (ret == STATUS_TIMEOUT && user_apc) ret = STATUS_USER_APC;
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=64582
Your paranoid android.
=== debian10 (32 bit Chinese:China report) ===
ntdll: pipe.c:1557: Test failed: pipe is not signaled pipe.c:1576: Test failed: pipe is not signaled
Signed-off-by: Jacek Caban jacek@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/ntdll/ntdll_misc.h | 2 +- dlls/ntdll/server.c | 14 ++++++-------- dlls/ntdll/sync.c | 5 +++-- 3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h index e1fb1e9eba7..bec88e6f07b 100644 --- a/dlls/ntdll/ntdll_misc.h +++ b/dlls/ntdll/ntdll_misc.h @@ -115,7 +115,7 @@ extern NTSTATUS alloc_object_attributes( const OBJECT_ATTRIBUTES *attr, struct o data_size_t *ret_len ) DECLSPEC_HIDDEN; extern NTSTATUS validate_open_object_attributes( const OBJECT_ATTRIBUTES *attr ) DECLSPEC_HIDDEN; extern int wait_select_reply( void *cookie ) DECLSPEC_HIDDEN; -extern BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) DECLSPEC_HIDDEN; +extern void invoke_apc( const apc_call_t *call, apc_result_t *result ) DECLSPEC_HIDDEN;
/* module handling */ extern LIST_ENTRY tls_links DECLSPEC_HIDDEN; diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 18ee41790f3..c17ab964d5d 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -384,11 +384,11 @@ int wait_select_reply( void *cookie ) /*********************************************************************** * invoke_apc * - * Invoke a single APC. Return TRUE if a user APC has been run. + * Invoke a single APC. + * */ -BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) +void invoke_apc( const apc_call_t *call, apc_result_t *result ) { - BOOL user_apc = FALSE; SIZE_T size; void *addr; pe_image_info_t image_info; @@ -403,7 +403,6 @@ BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) { void (WINAPI *func)(ULONG_PTR,ULONG_PTR,ULONG_PTR) = wine_server_get_ptr( call->user.func ); func( call->user.args[0], call->user.args[1], call->user.args[2] ); - user_apc = TRUE; break; } case APC_TIMER: @@ -411,7 +410,6 @@ BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) void (WINAPI *func)(void*, unsigned int, unsigned int) = wine_server_get_ptr( call->timer.func ); func( wine_server_get_ptr( call->timer.arg ), (DWORD)call->timer.time, (DWORD)(call->timer.time >> 32) ); - user_apc = TRUE; break; } case APC_ASYNC_IO: @@ -587,7 +585,6 @@ BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) server_protocol_error( "get_apc_request: bad type %d\n", call->type ); break; } - return user_apc; }
@@ -628,9 +625,10 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT) size = offsetof( select_op_t, signal_and_wait.signal );
- if ((ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC) && - invoke_apc( &call, &result )) + if (ret == STATUS_KERNEL_APC) invoke_apc( &call, &result ); + if (ret == STATUS_USER_APC) { + invoke_apc( &call, &result ); /* if we ran a user apc we have to check once more if additional apcs are queued, * but we don't want to wait */ abs_timeout = 0; diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 3b8b4f4eaaa..3e670f47f78 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -2496,9 +2496,10 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
RtlLeaveCriticalSection( &addr_section );
- if ((ret == STATUS_USER_APC || ret == STATUS_KERNEL_APC) && - invoke_apc( &call, &result )) + if (ret == STATUS_KERNEL_APC) invoke_apc( &call, &result ); + if (ret == STATUS_USER_APC) { + invoke_apc( &call, &result ); /* if we ran a user apc we have to check once more if additional apcs are queued, * but we don't want to wait */ abs_timeout = 0;
Signed-off-by: Jacek Caban jacek@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/ntdll/server.c | 39 ++++++++++++++++++++++----------------- dlls/ntdll/sync.c | 31 ++++++++++++++++++------------- 2 files changed, 40 insertions(+), 30 deletions(-)
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index c17ab964d5d..15d53b348ef 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -606,26 +606,31 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT
do { - SERVER_START_REQ( select ) + for (;;) { - req->flags = flags; - req->cookie = wine_server_client_ptr( &cookie ); - req->prev_apc = apc_handle; - req->timeout = abs_timeout; - wine_server_add_data( req, &result, sizeof(result) ); - wine_server_add_data( req, select_op, size ); - ret = wine_server_call( req ); - abs_timeout = reply->timeout; - apc_handle = reply->apc_handle; - call = reply->call; - } - SERVER_END_REQ; + SERVER_START_REQ( select ) + { + req->flags = flags; + req->cookie = wine_server_client_ptr( &cookie ); + req->prev_apc = apc_handle; + req->timeout = abs_timeout; + wine_server_add_data( req, &result, sizeof(result) ); + wine_server_add_data( req, select_op, size ); + ret = wine_server_call( req ); + abs_timeout = reply->timeout; + apc_handle = reply->apc_handle; + call = reply->call; + } + SERVER_END_REQ; + + /* don't signal multiple times */ + if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT) + size = offsetof( select_op_t, signal_and_wait.signal );
- /* don't signal multiple times */ - if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT) - size = offsetof( select_op_t, signal_and_wait.signal ); + if (ret != STATUS_KERNEL_APC) break; + invoke_apc( &call, &result ); + }
- if (ret == STATUS_KERNEL_APC) invoke_apc( &call, &result ); if (ret == STATUS_USER_APC) { invoke_apc( &call, &result ); diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 3e670f47f78..8b12e03eeff 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -2479,24 +2479,29 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size return STATUS_SUCCESS; }
- SERVER_START_REQ( select ) + for (;;) { - req->flags = SELECT_INTERRUPTIBLE; - req->cookie = wine_server_client_ptr( &cookie ); - req->prev_apc = apc_handle; - req->timeout = abs_timeout; - wine_server_add_data( req, &result, sizeof(result) ); - wine_server_add_data( req, &select_op, sizeof(select_op.keyed_event) ); - ret = wine_server_call( req ); - abs_timeout = reply->timeout; - apc_handle = reply->apc_handle; - call = reply->call; + SERVER_START_REQ( select ) + { + req->flags = SELECT_INTERRUPTIBLE; + req->cookie = wine_server_client_ptr( &cookie ); + req->prev_apc = apc_handle; + req->timeout = abs_timeout; + wine_server_add_data( req, &result, sizeof(result) ); + wine_server_add_data( req, &select_op, sizeof(select_op.keyed_event) ); + ret = wine_server_call( req ); + abs_timeout = reply->timeout; + apc_handle = reply->apc_handle; + call = reply->call; + } + SERVER_END_REQ; + + if (ret != STATUS_KERNEL_APC) break; + invoke_apc( &call, &result ); } - SERVER_END_REQ;
RtlLeaveCriticalSection( &addr_section );
- if (ret == STATUS_KERNEL_APC) invoke_apc( &call, &result ); if (ret == STATUS_USER_APC) { invoke_apc( &call, &result );
Signed-off-by: Jacek Caban jacek@codeweavers.com
This makes sure that system APC, such as APC_BREAK_PROCESS do not get interrupted in the middle of their execution, and that the APC completion notification is always correctly sent back to the caller.
Otherwise DbgBreakProcess sometimes did not return until WaitForDebugEvent/ContinueDebugEvent are called, because of a race condition between the APC servicing thread, and the newly created exception thread.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/ntdll/server.c | 5 ++++- dlls/ntdll/sync.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index 15d53b348ef..4facdc08a72 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -601,11 +601,13 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT apc_call_t call; apc_result_t result; timeout_t abs_timeout = timeout ? timeout->QuadPart : TIMEOUT_INFINITE; + sigset_t old_set;
memset( &result, 0, sizeof(result) );
do { + pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); for (;;) { SERVER_START_REQ( select ) @@ -616,7 +618,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT req->timeout = abs_timeout; wine_server_add_data( req, &result, sizeof(result) ); wine_server_add_data( req, select_op, size ); - ret = wine_server_call( req ); + ret = server_call_unlocked( req ); abs_timeout = reply->timeout; apc_handle = reply->apc_handle; call = reply->call; @@ -630,6 +632,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT if (ret != STATUS_KERNEL_APC) break; invoke_apc( &call, &result ); } + pthread_sigmask( SIG_SETMASK, &old_set, NULL );
if (ret == STATUS_USER_APC) { diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 8b12e03eeff..2b5b6ce44a5 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -2457,6 +2457,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size apc_call_t call; apc_result_t result; timeout_t abs_timeout = timeout ? timeout->QuadPart : TIMEOUT_INFINITE; + sigset_t old_set;
if (size != 1 && size != 2 && size != 4 && size != 8) return STATUS_INVALID_PARAMETER; @@ -2479,6 +2480,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size return STATUS_SUCCESS; }
+ pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); for (;;) { SERVER_START_REQ( select ) @@ -2489,7 +2491,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size req->timeout = abs_timeout; wine_server_add_data( req, &result, sizeof(result) ); wine_server_add_data( req, &select_op, sizeof(select_op.keyed_event) ); - ret = wine_server_call( req ); + ret = server_call_unlocked( req ); abs_timeout = reply->timeout; apc_handle = reply->apc_handle; call = reply->call; @@ -2499,6 +2501,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size if (ret != STATUS_KERNEL_APC) break; invoke_apc( &call, &result ); } + pthread_sigmask( SIG_SETMASK, &old_set, NULL );
RtlLeaveCriticalSection( &addr_section );
Signed-off-by: Jacek Caban jacek@codeweavers.com
-
Jacek Caban -
Marvin -
Rémi Bernon