This fixes the send part of the issue #52401 [1]: "Improper synchronization in sock_recv/sock_send leads to arbitrary reordering of completion of I/O requests", which was initially reported (outside Bugzilla) by Dongwan Kim [2].
Changelog: v1 -> v2: - drop patch "server: Compact struct set_async_direct_result_reply." - add async and fd arguments to async_initial_status_callback - recv_socket_initial_callback: pass OOB flag via private instead of implementing two separate functions for OOB and non-OOB - detect short write in send_socket_initial_callback - preserve the behaviour of returning success if we had a short write and the socket is nonblocking and force_async is unset v2 -> v3: - fix typo in comment
[1] https://bugs.winehq.org/show_bug.cgi?id=52401 [2] https://www.winehq.org/pipermail/wine-devel/2021-May/186454.html
Jinoh Kang (8): server: Actually set initial status in set_async_direct_result handler. server: Add async_initial_status_callback. server: Defer postprocessing until after setting initial status in recv_socket handler. server: Defer postprocessing until after setting initial status in send_socket handler. server: Add mark_pending field to set_async_direct_result request. server: Attempt to complete I/O request immediately in send_socket. ntdll: Don't call try_send before server call in sock_send. server: Replace redundant send_socket status fields with force_async boolean field.
dlls/ntdll/unix/socket.c | 102 ++++++++++++++++++++++-------- dlls/ntdll/unix/sync.c | 9 +-- dlls/ntdll/unix/unix_private.h | 2 +- dlls/ws2_32/tests/sock.c | 14 ++--- server/async.c | 52 +++++++++++++--- server/file.h | 2 + server/protocol.def | 6 +- server/sock.c | 110 +++++++++++++++++++-------------- 8 files changed, 204 insertions(+), 93 deletions(-)
Interdiff: diff --git a/server/sock.c b/server/sock.c index 2706c2d20ed..1112708369d 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3513,7 +3513,7 @@ DECL_HANDLER(send_socket) else if (!async_queued( &sock->write_q )) { /* If write_q is not empty, we cannot really tell if the already queued - * asyncs will not consume all available data; if there's no data + * asyncs will not consume all available space; if there's no space * available, the current request won't be immediately satiable. */ struct pollfd pollfd;
Commit 15483b1a126 (server: Allow calling async_handoff() with status code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result handler which calls async_set_initial_status().
However, the async_set_initial_status() call does nothing since async->terminated is set, leaving the async in a confusing state (unknown_status = 1 but pending/completed).
So far, this issue is unlikely to have been a problem in practice for the following reasons:
1. async_set_initial_status() would have unset unknown_status, but it remains set instead. This is usually not a problem, since unknown_status is usually ever read by code paths effectively unreachable for non-device (e.g. socket) asyncs.
It would still potentially allow set_async_direct_result to be called multiple times, but it wouldn't actually happen in practice unless something goes wrong.
2. async_set_initial_status() would have set initial_status; however, it is left with the default value STATUS_PENDING. If the actual status is something other than that, the handler closes the wait handle and async_satisfied (the only realconsumer of initial_status) would never be called anyway.
For reasons above, this issue is not effectively observable or testable. Nonetheless, the current code does leave the async object in an inconsistent state.
Fix this by setting the initial_status directly (and unsetting unknown_status as well).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes
server/async.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/server/async.c b/server/async.c index 7d0ff10f7a4..f078f42f34c 100644 --- a/server/async.c +++ b/server/async.c @@ -769,7 +769,8 @@ DECL_HANDLER(set_async_direct_result) return; }
- async_set_initial_status( async, status ); + async->initial_status = status; + async->unknown_status = 0;
if (status == STATUS_PENDING) {
On 2/21/22 11:23, Jinoh Kang wrote:
Commit 15483b1a126 (server: Allow calling async_handoff() with status code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result handler which calls async_set_initial_status().
However, the async_set_initial_status() call does nothing since async->terminated is set, leaving the async in a confusing state (unknown_status = 1 but pending/completed).
So far, this issue is unlikely to have been a problem in practice for the following reasons:
async_set_initial_status() would have unset unknown_status, but it remains set instead. This is usually not a problem, since unknown_status is usually ever read by code paths effectively unreachable for non-device (e.g. socket) asyncs.
It would still potentially allow set_async_direct_result to be called multiple times, but it wouldn't actually happen in practice unless something goes wrong.
async_set_initial_status() would have set initial_status; however, it is left with the default value STATUS_PENDING. If the actual status is something other than that, the handler closes the wait handle and async_satisfied (the only realconsumer of initial_status) would never be called anyway.
For reasons above, this issue is not effectively observable or testable. Nonetheless, the current code does leave the async object in an inconsistent state.
Fix this by setting the initial_status directly (and unsetting unknown_status as well).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Notes: v1 -> v2: no changes v2 -> v3: no changes
server/async.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/server/async.c b/server/async.c index 7d0ff10f7a4..f078f42f34c 100644 --- a/server/async.c +++ b/server/async.c @@ -769,7 +769,8 @@ DECL_HANDLER(set_async_direct_result) return; }
- async_set_initial_status( async, status );
async->initial_status = status;
async->unknown_status = 0;
if (status == STATUS_PENDING) {
I feel like this raises further questions, like "why does async_set_initial_status() assert async->unknown_status but then return if async->terminated?"
For IRPs I think the async can be terminated with unknown_status if the process dies. I'm not sure if we have an actual bug here, but conceptually we should probably be resetting unknown_status and setting initial_status in async_set_result() if necessary.
Of course, if we do that, I think it becomes possible to call async_set_initial_status() on an async which already has !async->unknown_status, so that assert() would have to go away.
On 2/26/22 03:57, Zebediah Figura (she/her) wrote:
On 2/21/22 11:23, Jinoh Kang wrote:
Commit 15483b1a126 (server: Allow calling async_handoff() with status code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result handler which calls async_set_initial_status().
However, the async_set_initial_status() call does nothing since async->terminated is set, leaving the async in a confusing state (unknown_status = 1 but pending/completed).
So far, this issue is unlikely to have been a problem in practice for the following reasons:
async_set_initial_status() would have unset unknown_status, but it remains set instead. This is usually not a problem, since unknown_status is usually ever read by code paths effectively unreachable for non-device (e.g. socket) asyncs.
It would still potentially allow set_async_direct_result to be called multiple times, but it wouldn't actually happen in practice unless something goes wrong.
async_set_initial_status() would have set initial_status; however, it is left with the default value STATUS_PENDING. If the actual status is something other than that, the handler closes the wait handle and async_satisfied (the only realconsumer of initial_status) would never be called anyway.
For reasons above, this issue is not effectively observable or testable. Nonetheless, the current code does leave the async object in an inconsistent state.
Fix this by setting the initial_status directly (and unsetting unknown_status as well).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Notes: v1 -> v2: no changes v2 -> v3: no changes
server/async.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/server/async.c b/server/async.c index 7d0ff10f7a4..f078f42f34c 100644 --- a/server/async.c +++ b/server/async.c @@ -769,7 +769,8 @@ DECL_HANDLER(set_async_direct_result) return; }
- async_set_initial_status( async, status );
async->initial_status = status;
async->unknown_status = 0;
if (status == STATUS_PENDING) {
I feel like this raises further questions, like "why does async_set_initial_status() assert async->unknown_status
To prevent overwriting initial status that was already set, I guess?
but then return if async->terminated?"
This function was introduced in commit 484b78bda01 (ntoskrnl: Report the initial status of an IRP separately from the IOSB status., 2021-09-12), but it provides no explanation.
I guess it was to keep it in sync with set_async_pending()?
That said, I suppose nothing will go wrong even if we pull out the !async->terminated check. Nobody really cares about the initial status after termination, except async_satisfied().
For IRPs I think the async can be terminated with unknown_status if the process dies. I'm not sure if we have an actual bug here,
This is not a bug (yet), but subsequent patches in this series require that the initial status is eventually set.
but conceptually we should probably be resetting unknown_status and setting initial_status in async_set_result() if necessary.
Great.
Of course, if we do that, I think it becomes possible to call async_set_initial_status() on an async which already has !async->unknown_status, so that assert() would have to go away.
Yeah, and also the !async->terminated check.
On 3/1/22 07:23, Jinoh Kang wrote:
On 2/26/22 03:57, Zebediah Figura (she/her) wrote:
On 2/21/22 11:23, Jinoh Kang wrote:
Commit 15483b1a126 (server: Allow calling async_handoff() with status code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result handler which calls async_set_initial_status().
However, the async_set_initial_status() call does nothing since async->terminated is set, leaving the async in a confusing state (unknown_status = 1 but pending/completed).
So far, this issue is unlikely to have been a problem in practice for the following reasons:
async_set_initial_status() would have unset unknown_status, but it remains set instead. This is usually not a problem, since unknown_status is usually ever read by code paths effectively unreachable for non-device (e.g. socket) asyncs.
It would still potentially allow set_async_direct_result to be called multiple times, but it wouldn't actually happen in practice unless something goes wrong.
async_set_initial_status() would have set initial_status; however, it is left with the default value STATUS_PENDING. If the actual status is something other than that, the handler closes the wait handle and async_satisfied (the only realconsumer of initial_status) would never be called anyway.
For reasons above, this issue is not effectively observable or testable. Nonetheless, the current code does leave the async object in an inconsistent state.
Fix this by setting the initial_status directly (and unsetting unknown_status as well).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Notes: v1 -> v2: no changes v2 -> v3: no changes
server/async.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/server/async.c b/server/async.c index 7d0ff10f7a4..f078f42f34c 100644 --- a/server/async.c +++ b/server/async.c @@ -769,7 +769,8 @@ DECL_HANDLER(set_async_direct_result) return; }
- async_set_initial_status( async, status );
async->initial_status = status;
async->unknown_status = 0;
if (status == STATUS_PENDING) {
I feel like this raises further questions, like "why does async_set_initial_status() assert async->unknown_status
To prevent overwriting initial status that was already set, I guess?
but then return if async->terminated?"
This function was introduced in commit 484b78bda01 (ntoskrnl: Report the initial status of an IRP separately from the IOSB status., 2021-09-12), but it provides no explanation.
I guess it was to keep it in sync with set_async_pending()?
Broadly, or, put another way, I think the answer is "the code was already like that" :-)
That said, I suppose nothing will go wrong even if we pull out the !async->terminated check. Nobody really cares about the initial status after termination, except async_satisfied().
Right, and anyway I don't think there's any reason that we should care here.
For IRPs I think the async can be terminated with unknown_status if the process dies. I'm not sure if we have an actual bug here,
This is not a bug (yet), but subsequent patches in this series require that the initial status is eventually set.
but conceptually we should probably be resetting unknown_status and setting initial_status in async_set_result() if necessary.
Great.
Of course, if we do that, I think it becomes possible to call async_set_initial_status() on an async which already has !async->unknown_status, so that assert() would have to go away.
Yeah, and also the !async->terminated check.
Right, that's my thinking too :-)
async->initial_status_callback is called whenever the initial_status is set. This can be used to do the post-processing after the initial I/O has been performed by the client, but before the async is actually completed or queued.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - add spacing around initial_status_callback call arguments - add async and fd arguments to async_initial_status_callback v2 -> v3: no changes
server/async.c | 40 +++++++++++++++++++++++++++++++++++----- server/file.h | 2 ++ 2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/server/async.c b/server/async.c index f078f42f34c..d597eb71f12 100644 --- a/server/async.c +++ b/server/async.c @@ -62,6 +62,8 @@ struct async unsigned int comp_flags; /* completion flags */ async_completion_callback completion_callback; /* callback to be called on completion */ void *completion_callback_private; /* argument to completion_callback */ + async_initial_status_callback initial_status_callback; /* callback to be called on initial status set */ + void *initial_status_callback_private; /* argument to initial_status_callback */ };
static void async_dump( struct object *obj, int verbose ); @@ -139,11 +141,24 @@ static void async_satisfied( struct object *obj, struct wait_queue_entry *entry } }
+static void set_async_initial_status( struct async *async, unsigned int status ) +{ + async->initial_status = status; + if (async->initial_status_callback) + { + async->initial_status_callback( async->initial_status_callback_private, async, async->fd, status ); + async->initial_status_callback = NULL; + } +} + static void async_destroy( struct object *obj ) { struct async *async = (struct async *)obj; assert( obj->ops == &async_ops );
+ /* ensure initial_status_callback has been called */ + set_async_initial_status( async, async->initial_status ); + list_remove( &async->process_entry );
if (async->queue) @@ -281,6 +296,8 @@ struct async *create_async( struct fd *fd, struct thread *thread, const async_da async->comp_flags = 0; async->completion_callback = NULL; async->completion_callback_private = NULL; + async->initial_status_callback = NULL; + async->initial_status_callback_private = NULL;
if (iosb) async->iosb = (struct iosb *)grab_object( iosb ); else async->iosb = NULL; @@ -298,6 +315,13 @@ struct async *create_async( struct fd *fd, struct thread *thread, const async_da return async; }
+/* set a callback to be notified when the async initial status is set */ +void async_set_initial_status_callback( struct async *async, async_initial_status_callback func, void *private ) +{ + async->initial_status_callback = func; + async->initial_status_callback_private = private; +} + /* set the initial status of an async whose status was previously unknown * the initial status may be STATUS_PENDING */ void async_set_initial_status( struct async *async, unsigned int status ) @@ -305,7 +329,7 @@ void async_set_initial_status( struct async *async, unsigned int status ) assert( async->unknown_status ); if (!async->terminated) { - async->initial_status = status; + set_async_initial_status( async, status ); async->unknown_status = 0; } } @@ -363,7 +387,7 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ return async->wait_handle; }
- async->initial_status = get_error(); + set_async_initial_status( async, get_error() );
if (!async->pending && NT_ERROR( get_error() )) { @@ -402,7 +426,7 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ async->wait_handle = 0; } } - async->initial_status = async->iosb->status; + set_async_initial_status( async, async->iosb->status ); set_error( async->iosb->status ); return async->wait_handle; } @@ -759,9 +783,13 @@ DECL_HANDLER(set_async_direct_result) { struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); unsigned int status = req->status; + apc_param_t information = req->information;
if (!async) return;
+ /* we only return an async handle for asyncs created via create_request_async() */ + assert( async->iosb ); + if (!async->unknown_status || !async->terminated || !async->alerted) { set_error( STATUS_INVALID_PARAMETER ); @@ -769,7 +797,9 @@ DECL_HANDLER(set_async_direct_result) return; }
- async->initial_status = status; + /* Set iosb information field for async_initial_status_callback */ + async->iosb->result = information; + set_async_initial_status( async, status ); async->unknown_status = 0;
if (status == STATUS_PENDING) @@ -782,7 +812,7 @@ DECL_HANDLER(set_async_direct_result) * set the IOSB. therefore, we can skip waiting on wait_handle and do * async_set_result() directly. */ - async_set_result( &async->obj, status, req->information ); + async_set_result( &async->obj, status, information );
/* close wait handle here to avoid extra server round trip, if the I/O * either has completed, or is pending and not blocking. diff --git a/server/file.h b/server/file.h index 9f9d4cd4e1a..893d4ac6411 100644 --- a/server/file.h +++ b/server/file.h @@ -219,6 +219,7 @@ extern struct object *create_serial( struct fd *fd ); /* async I/O functions */
typedef void (*async_completion_callback)( void *private ); +typedef void (*async_initial_status_callback)( void *private, struct async *async, struct fd *fd, unsigned int status );
extern void free_async_queue( struct async_queue *queue ); extern struct async *create_async( struct fd *fd, struct thread *thread, const async_data_t *data, struct iosb *iosb ); @@ -230,6 +231,7 @@ extern void async_set_result( struct object *obj, unsigned int status, apc_param extern void async_set_completion_callback( struct async *async, async_completion_callback func, void *private ); extern void async_set_unknown_status( struct async *async ); extern void set_async_pending( struct async *async ); +extern void async_set_initial_status_callback( struct async *async, async_initial_status_callback func, void *private ); extern void async_set_initial_status( struct async *async, unsigned int status ); extern void async_wake_obj( struct async *async ); extern int async_waiting( struct async_queue *queue );
This allows the client to postpone the initial I/O until after the server has actually queued the I/O request. The server should perform the postprocessing only after the initial I/O has been done.
In the case of recv_socket, the manipulation of event flags shall ideally be done *after* (not *before*) the client has attempted the initial I/O, since the readiness of the socket may change in the meanwhile.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - recv_socket_initial_callback: pass OOB flag via private instead of implementing two separate functions for OOB and non-OOB v2 -> v3: no changes
server/sock.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 91f98556552..f6a65e4e084 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3393,6 +3393,16 @@ struct object *create_socket_device( struct object *root, const struct unicode_s return create_named_object( root, &socket_device_ops, name, attr, sd ); }
+static void recv_socket_initial_callback( void *private, struct async *async, struct fd *fd, unsigned int status ) +{ + struct sock *sock = get_fd_user( fd ); + int is_oob = (int)(long)private; + + sock->pending_events &= ~(is_oob ? AFD_POLL_OOB : AFD_POLL_READ); + sock->reported_events &= ~(is_oob ? AFD_POLL_OOB : AFD_POLL_READ); + sock_reselect( sock ); +} + DECL_HANDLER(recv_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); @@ -3430,22 +3440,18 @@ DECL_HANDLER(recv_socket) if (status == STATUS_PENDING && !req->force_async && sock->nonblocking) status = STATUS_DEVICE_NOT_READY;
- sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); - sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); - if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { set_error( status );
+ async_set_initial_status_callback( async, recv_socket_initial_callback, (void *)(long)req->oob ); + if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
if (status == STATUS_PENDING || status == STATUS_ALERTED) queue_async( &sock->read_q, async );
- /* always reselect; we changed reported_events above */ - sock_reselect( sock ); - reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); reply->nonblocking = sock->nonblocking;
On 2/21/22 11:23, Jinoh Kang wrote:
This allows the client to postpone the initial I/O until after the server has actually queued the I/O request. The server should perform the postprocessing only after the initial I/O has been done.
In the case of recv_socket, the manipulation of event flags shall ideally be done *after* (not *before*) the client has attempted the initial I/O, since the readiness of the socket may change in the meanwhile.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
What's the point of doing this?
On 2/26/22 03:42, Zebediah Figura (she/her) wrote:
On 2/21/22 11:23, Jinoh Kang wrote:
This allows the client to postpone the initial I/O until after the server has actually queued the I/O request. The server should perform the postprocessing only after the initial I/O has been done.
In the case of recv_socket, the manipulation of event flags shall ideally be done *after* (not *before*) the client has attempted the initial I/O, since the readiness of the socket may change in the meanwhile.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
What's the point of doing this?
Clarity, mostly.
Clearing pending_events indicates that we're interested in polling I/O again. Since the recv() call has not yet happened, it means the incoming queue may still have pending data. One might reason that this will erroneously trigger AFD_POLL_READ event, even though the recv() call will consume all the remaining data.
Now, the above scenario won't really happen since we never poll for POLLIN if async_waiting( &sock->read_q ) is false. I nonetheless find it much more intuitive to have the event flag cleared *after* performing the initial I/O. This also makes it consistent with the send_socket case.
On 3/1/22 06:42, Jinoh Kang wrote:
On 2/26/22 03:42, Zebediah Figura (she/her) wrote:
On 2/21/22 11:23, Jinoh Kang wrote:
This allows the client to postpone the initial I/O until after the server has actually queued the I/O request. The server should perform the postprocessing only after the initial I/O has been done.
In the case of recv_socket, the manipulation of event flags shall ideally be done *after* (not *before*) the client has attempted the initial I/O, since the readiness of the socket may change in the meanwhile.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
What's the point of doing this?
Clarity, mostly.
Clearing pending_events indicates that we're interested in polling I/O again. Since the recv() call has not yet happened, it means the incoming queue may still have pending data. One might reason that this will erroneously trigger AFD_POLL_READ event, even though the recv() call will consume all the remaining data.
Now, the above scenario won't really happen since we never poll for POLLIN if async_waiting( &sock->read_q ) is false. I nonetheless find it much more intuitive to have the event flag cleared *after* performing the initial I/O. This also makes it consistent with the send_socket case.
Well, we can't clear event flags after performing the initial I/O per se. At best we can do so after *queuing* it, which isn't particularly meaningful. Perhaps there's an argument for moving the flag manipulation after queue_async(), but I'll admit it doesn't really seem worthwhile. I don't see the point of moving it after async_handoff(), though.
Note also that we have to clear event flags even in (some?) error cases, where no I/O is really queued. So while I can only assume this passes the tests, it makes it much less obvious that it's the right thing to do. It also introduces a mental burden on the reader to understand the semantics of the "initial status callback" and when (and under what circumstances) it's called.
This allows the client to postpone the initial I/O until the server has queued the I/O request. The server should perform the postprocessing only after the initial I/O has been done.
In the case of send_socket, the manipulation of event flags shall ideally be done *after* (not *before*) the client has attempted the initial I/O, since the outbound queue status of the socket may change in the meanwhile. Also, the implicitly bound address is available only after the send* system call has been performed.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - pass around total size of data to be transmitted - detect short write in send_socket_initial_callback v2 -> v3: no changes
dlls/ntdll/unix/socket.c | 15 ++++++++++++++ server/protocol.def | 1 + server/sock.c | 42 +++++++++++++++++++++++++++------------- 3 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 23059e3cff8..d04ebb7f874 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -873,6 +873,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi NTSTATUS status; unsigned int i; ULONG options; + data_size_t data_size;
async_size = offsetof( struct async_send_ioctl, iov[count] );
@@ -906,6 +907,18 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi async->iov_cursor = 0; async->sent_len = 0;
+ data_size = 0; + for (i = 0; i < count; ++i) + { + SIZE_T len = async->iov[i].iov_len; + if (len > (SIZE_T)(data_size_t)-1 || (data_size_t)(data_size + len) < data_size) + { + release_fileio( &async->io ); + return STATUS_NO_MEMORY; + } + data_size += len; + } + status = try_send( fd, async );
if (status != STATUS_SUCCESS && status != STATUS_DEVICE_NOT_READY) @@ -919,6 +932,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
SERVER_START_REQ( send_socket ) { + req->data_size = data_size; req->status = status; req->total = async->sent_len; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); @@ -1099,6 +1113,7 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc,
SERVER_START_REQ( send_socket ) { + req->data_size = async->head_len + async->file_len + async->tail_len; req->status = STATUS_PENDING; req->total = 0; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); diff --git a/server/protocol.def b/server/protocol.def index 9d90544fa41..2e57c91ccad 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1461,6 +1461,7 @@ enum server_fd_type
/* Perform a send on a socket */ @REQ(send_socket) + data_size_t data_size; /* total number of bytes to send (>= total) */ async_data_t async; /* async I/O parameters */ unsigned int status; /* status of initial call */ unsigned int total; /* number of bytes already sent */ diff --git a/server/sock.c b/server/sock.c index f6a65e4e084..8653492ce34 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3460,16 +3460,17 @@ DECL_HANDLER(recv_socket) release_object( sock ); }
-DECL_HANDLER(send_socket) +static void send_socket_initial_callback( void *private, struct async *async, struct fd *fd, unsigned int status ) { - struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); - unsigned int status = req->status; - timeout_t timeout = 0; - struct async *async; - struct fd *fd; + struct sock *sock = get_fd_user( fd ); + struct iosb *iosb; + int is_short_write = 0;
- if (!sock) return; - fd = sock->fd; + if ((iosb = async_get_iosb( async ))) + { + is_short_write = iosb->result < (unsigned long)private; + release_object( iosb ); + }
if (sock->type == WS_SOCK_DGRAM) { @@ -3482,13 +3483,29 @@ DECL_HANDLER(send_socket) sock->bound = 1; }
- if (status != STATUS_SUCCESS) + if (status != STATUS_SUCCESS || is_short_write) { - /* send() calls only clear and reselect events if unsuccessful. */ + /* send() calls only clear and reselect events if unsuccessful. + * Also treat short writes as being unsuccessful. + */ sock->pending_events &= ~AFD_POLL_WRITE; sock->reported_events &= ~AFD_POLL_WRITE; }
+ sock_reselect( sock ); +} + +DECL_HANDLER(send_socket) +{ + struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); + unsigned int status = req->status; + timeout_t timeout = 0; + struct async *async; + struct fd *fd; + + if (!sock) return; + fd = sock->fd; + /* If we had a short write and the socket is nonblocking (and the client is * not trying to force the operation to be asynchronous), return success. * Windows actually refuses to send any data in this case, and returns @@ -3524,15 +3541,14 @@ DECL_HANDLER(send_socket) } set_error( status );
+ async_set_initial_status_callback( async, send_socket_initial_callback, (void *)(unsigned long)req->data_size ); + if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
if (status == STATUS_PENDING) queue_async( &sock->write_q, async );
- /* always reselect; we changed reported_events above */ - sock_reselect( sock ); - reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); release_object( async );
The client can set mark_pending to indicate that the full-blown I/O completion mechanism shall be triggered (asynchronous completion) even if the status indicates failure.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes
dlls/ntdll/unix/socket.c | 2 +- dlls/ntdll/unix/sync.c | 9 +++++---- dlls/ntdll/unix/unix_private.h | 2 +- server/async.c | 11 ++++++++--- server/protocol.def | 1 + 5 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index d04ebb7f874..f24215b66a9 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -767,7 +767,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi release_fileio( &async->io ); }
- if (alerted) set_async_direct_result( &wait_handle, status, information ); + if (alerted) set_async_direct_result( &wait_handle, status, information, FALSE ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index ee25dfd0099..0786454dad2 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2514,7 +2514,7 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG /* Notify direct completion of async and close the wait handle if it is no longer needed. * This function is a no-op (returns status as-is) if the supplied handle is NULL. */ -void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information ) +void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending ) { NTSTATUS ret;
@@ -2522,9 +2522,10 @@ void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PT
SERVER_START_REQ( set_async_direct_result ) { - req->handle = wine_server_obj_handle( *optional_handle ); - req->status = status; - req->information = information; + req->handle = wine_server_obj_handle( *optional_handle ); + req->status = status; + req->information = information; + req->mark_pending = mark_pending; ret = wine_server_call( req ); if (ret == STATUS_SUCCESS) *optional_handle = wine_server_ptr_handle( reply->handle ); diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 75f03706401..b0458849adb 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -274,7 +274,7 @@ extern NTSTATUS get_device_info( int fd, struct _FILE_FS_DEVICE_INFORMATION *inf extern void init_files(void) DECLSPEC_HIDDEN; extern void init_cpu_info(void) DECLSPEC_HIDDEN; extern void add_completion( HANDLE handle, ULONG_PTR value, NTSTATUS status, ULONG info, BOOL async ) DECLSPEC_HIDDEN; -extern void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information ); +extern void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending );
extern void dbg_init(void) DECLSPEC_HIDDEN;
diff --git a/server/async.c b/server/async.c index d597eb71f12..ded4f8392d2 100644 --- a/server/async.c +++ b/server/async.c @@ -807,10 +807,15 @@ DECL_HANDLER(set_async_direct_result) async->direct_result = 0; async->pending = 1; } + else if (req->mark_pending) + { + async->pending = 1; + }
- /* if the I/O has completed successfully, the client would have already - * set the IOSB. therefore, we can skip waiting on wait_handle and do - * async_set_result() directly. + /* if the I/O has completed successfully (or unsuccessfully, and + * async->pending is set), the client would have already set the IOSB. + * therefore, we can do async_set_result() directly and let the client skip + * waiting on wait_handle. */ async_set_result( &async->obj, status, information );
diff --git a/server/protocol.def b/server/protocol.def index 2e57c91ccad..5eb63db3091 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2169,6 +2169,7 @@ enum message_type obj_handle_t handle; /* wait handle */ apc_param_t information; /* IO_STATUS_BLOCK Information */ unsigned int status; /* completion status */ + int mark_pending; /* set async to pending before completion? */ @REPLY obj_handle_t handle; /* wait handle, or NULL if closed */ @END
Make send_socket alert the async immediately if poll() call detects that there are incoming data in the socket, bypassing the wineserver's main polling loop.
For sock_transmit, we always mark the async as pending and set the IOSB (unless async allocation has failed).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - retain the behaviour of returning success if we had a short write and the socket is nonblocking and force_async is unset v2 -> v3: fix typo in comment
dlls/ntdll/unix/socket.c | 72 ++++++++++++++++++++++++++++++++++------ server/protocol.def | 1 + server/sock.c | 22 +++++++++++- 3 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index f24215b66a9..23014ceceaf 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -868,12 +868,14 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi const struct WS_sockaddr *addr, unsigned int addr_len, int unix_flags, int force_async ) { struct async_send_ioctl *async; + ULONG_PTR information; HANDLE wait_handle; DWORD async_size; NTSTATUS status; unsigned int i; ULONG options; data_size_t data_size; + BOOL nonblocking, alerted;
async_size = offsetof( struct async_send_ioctl, iov[count] );
@@ -939,16 +941,38 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; - if ((!NT_ERROR(status) || wait_handle) && status != STATUS_PENDING) + nonblocking = reply->nonblocking; + } + SERVER_END_REQ; + + alerted = status == STATUS_ALERTED; + if (alerted) + { + status = try_send( fd, async ); + if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) + status = STATUS_PENDING; + + /* If we had a short write and the socket is nonblocking (and we are + * not trying to force the operation to be asynchronous), return + * success. Windows actually refuses to send any data in this case, + * and returns EWOULDBLOCK, but we have no way of doing that. */ + if (status == STATUS_DEVICE_NOT_READY && async->sent_len) + status = STATUS_SUCCESS; + } + + if (status != STATUS_PENDING) + { + information = async->sent_len; + if (!NT_ERROR(status) || (wait_handle && !alerted)) { io->Status = status; - io->Information = async->sent_len; + io->Information = information; } + release_fileio( &async->io ); } - SERVER_END_REQ; - - if (status != STATUS_PENDING) release_fileio( &async->io ); + else information = 0;
+ if (alerted) set_async_direct_result( &wait_handle, status, information, FALSE ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } @@ -1069,7 +1093,9 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, socklen_t addr_len; HANDLE wait_handle; NTSTATUS status; + ULONG_PTR information; ULONG options; + BOOL alerted;
addr_len = sizeof(addr); if (getpeername( fd, &addr.addr, &addr_len ) != 0) @@ -1120,16 +1146,40 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; - /* In theory we'd fill the iosb here, as above in sock_send(), but it's - * actually currently impossible to get STATUS_SUCCESS. The server will - * either return STATUS_PENDING or an error code, and in neither case - * should the iosb be filled. */ - if (!status) FIXME( "Unhandled success status." ); } SERVER_END_REQ;
- if (status != STATUS_PENDING) release_fileio( &async->io ); + alerted = status == STATUS_ALERTED; + if (alerted) + { + status = try_transmit( fd, file_fd, async ); + if (status == STATUS_DEVICE_NOT_READY) + status = STATUS_PENDING; + }
+ if (status != STATUS_PENDING) + { + information = async->head_cursor + async->file_cursor + async->tail_cursor; + if (!NT_ERROR(status) || wait_handle) + { + io->Status = status; + io->Information = information; + } + release_fileio( &async->io ); + } + else information = 0; + + if (alerted) + { + set_async_direct_result( &wait_handle, status, information, TRUE ); + if (!(options & (FILE_SYNCHRONOUS_IO_ALERT | FILE_SYNCHRONOUS_IO_NONALERT))) + { + /* Pretend we always do async I/O. The client can always retrieve + * the actual I/O status via the IO_STATUS_BLOCK. + */ + status = STATUS_PENDING; + } + } if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } diff --git a/server/protocol.def b/server/protocol.def index 5eb63db3091..334abb66a77 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1468,6 +1468,7 @@ enum server_fd_type @REPLY obj_handle_t wait; /* handle to wait on for blocking send */ unsigned int options; /* device open options */ + int nonblocking; /* is socket non-blocking? */ @END
diff --git a/server/sock.c b/server/sock.c index 8653492ce34..3cb1b5f3dab 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3531,6 +3531,25 @@ DECL_HANDLER(send_socket) if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->wr_shutdown) status = STATUS_PIPE_DISCONNECTED;
+ if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->write_q )) + { + /* If write_q is not empty, we cannot really tell if the already queued + * asyncs will not consume all available space; if there's no space + * available, the current request won't be immediately satiable. + */ + struct pollfd pollfd; + pollfd.fd = get_unix_fd( sock->fd ); + pollfd.events = POLLOUT; + pollfd.revents = 0; + if (poll(&pollfd, 1, 0) >= 0 && pollfd.revents) + { + /* Give the client opportunity to complete synchronously. + * If it turns out that the I/O request is not actually immediately satiable, + * the client may then choose to re-queue the async (with STATUS_PENDING). */ + status = STATUS_ALERTED; + } + } + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { if (status == STATUS_SUCCESS) @@ -3546,11 +3565,12 @@ DECL_HANDLER(send_socket) if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
- if (status == STATUS_PENDING) + if (status == STATUS_PENDING || status == STATUS_ALERTED) queue_async( &sock->write_q, async );
reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); + reply->nonblocking = sock->nonblocking; release_object( async ); } release_object( sock );
Otherwise, try_send() call from sock_send() may race against try_send() call from async_send_proc(), shuffling the packet order.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52401 Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes
dlls/ntdll/unix/socket.c | 13 ++----------- dlls/ws2_32/tests/sock.c | 14 +++++++------- 2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 23014ceceaf..ad03907523d 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -921,22 +921,13 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi data_size += len; }
- status = try_send( fd, async ); - - if (status != STATUS_SUCCESS && status != STATUS_DEVICE_NOT_READY) - { - release_fileio( &async->io ); - return status; - } - - if (status == STATUS_DEVICE_NOT_READY && force_async) - status = STATUS_PENDING; + status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
SERVER_START_REQ( send_socket ) { req->data_size = data_size; req->status = status; - req->total = async->sent_len; + req->total = 0; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 6b6267dc153..caf76868f89 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -7799,7 +7799,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -7846,7 +7846,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(server, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); @@ -7896,7 +7896,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -7914,7 +7914,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(server, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); @@ -8045,7 +8045,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = sendto(client, "test", 5, 0, (struct sockaddr *)&server_addr, sizeof(server_addr)); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
closesocket(client); closesocket(server); @@ -8126,7 +8126,7 @@ static void test_DisconnectEx(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "expected failure\n"); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -8180,7 +8180,7 @@ static void test_DisconnectEx(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "expected failure\n"); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret);
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=108821
Your paranoid android.
=== debian11 (32 bit WoW report) ===
Report validation errors: sock: Timeout
=== debian11 (build log) ===
WineRunWineTest.pl:error: The task timed out
The 'status' field of send_socket_request is always either STATUS_PENDING or STATUS_DEVICE_NOT_READY, and the 'total' field is always zero.
Replace the 'status' field with 'force_async' boolean field, and get rid of the 'total' field entirely.
Also, clean up the send_socket handler code a bit.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - adjust patch for new field "data_size" in send_socket_request v2 -> v3: no changes
dlls/ntdll/unix/socket.c | 8 ++------ server/protocol.def | 5 ++--- server/sock.c | 40 ++++++++-------------------------------- 3 files changed, 12 insertions(+), 41 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index ad03907523d..11ce652fed5 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -921,13 +921,10 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi data_size += len; }
- status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY; - SERVER_START_REQ( send_socket ) { req->data_size = data_size; - req->status = status; - req->total = 0; + req->force_async = force_async; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); @@ -1131,8 +1128,7 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, SERVER_START_REQ( send_socket ) { req->data_size = async->head_len + async->file_len + async->tail_len; - req->status = STATUS_PENDING; - req->total = 0; + req->force_async = 1; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); diff --git a/server/protocol.def b/server/protocol.def index 334abb66a77..5d8038d58b9 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1461,10 +1461,9 @@ enum server_fd_type
/* Perform a send on a socket */ @REQ(send_socket) - data_size_t data_size; /* total number of bytes to send (>= total) */ + data_size_t data_size; /* total number of bytes to send */ async_data_t async; /* async I/O parameters */ - unsigned int status; /* status of initial call */ - unsigned int total; /* number of bytes already sent */ + int force_async; /* Force asynchronous mode? */ @REPLY obj_handle_t wait; /* handle to wait on for blocking send */ unsigned int options; /* device open options */ diff --git a/server/sock.c b/server/sock.c index 3cb1b5f3dab..1112708369d 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3498,7 +3498,7 @@ static void send_socket_initial_callback( void *private, struct async *async, st DECL_HANDLER(send_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); - unsigned int status = req->status; + unsigned int status = STATUS_PENDING; timeout_t timeout = 0; struct async *async; struct fd *fd; @@ -3506,32 +3506,11 @@ DECL_HANDLER(send_socket) if (!sock) return; fd = sock->fd;
- /* If we had a short write and the socket is nonblocking (and the client is - * not trying to force the operation to be asynchronous), return success. - * Windows actually refuses to send any data in this case, and returns - * EWOULDBLOCK, but we have no way of doing that. */ - if (status == STATUS_DEVICE_NOT_READY && req->total && sock->nonblocking) - status = STATUS_SUCCESS; + if (!req->force_async && !sock->nonblocking && is_fd_overlapped( fd )) + timeout = (timeout_t)sock->sndtimeo * -10000;
- /* send() returned EWOULDBLOCK or a short write, i.e. cannot send all data yet */ - if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking) - { - /* Set a timeout on the async if necessary. - * - * We want to do this *only* if the client gave us STATUS_DEVICE_NOT_READY. - * If the client gave us STATUS_PENDING, it expects the async to always - * block (it was triggered by WSASend*() with a valid OVERLAPPED - * structure) and for the timeout not to be respected. */ - if (is_fd_overlapped( fd )) - timeout = (timeout_t)sock->sndtimeo * -10000; - - status = STATUS_PENDING; - } - - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->wr_shutdown) - status = STATUS_PIPE_DISCONNECTED; - - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->write_q )) + if (sock->wr_shutdown) status = STATUS_PIPE_DISCONNECTED; + else if (!async_queued( &sock->write_q )) { /* If write_q is not empty, we cannot really tell if the already queued * asyncs will not consume all available space; if there's no space @@ -3550,14 +3529,11 @@ DECL_HANDLER(send_socket) } }
+ if (status == STATUS_PENDING && !req->force_async && sock->nonblocking) + status = STATUS_DEVICE_NOT_READY; + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { - if (status == STATUS_SUCCESS) - { - struct iosb *iosb = async_get_iosb( async ); - iosb->result = req->total; - release_object( iosb ); - } set_error( status );
async_set_initial_status_callback( async, send_socket_initial_callback, (void *)(unsigned long)req->data_size );
-
Jinoh Kang -
Marvin -
Zebediah Figura (she/her)