[PATCH v4 0/1] MR10424: services: Cancel overlapped IO after timeout.
Though WaitForSingleObject timed out, the IO operation is still running in the background. After the function process_send_command returns, the OVERLAPPED object referenced by this IO operation becomes invalid. Later, when this IO finally completes, server_select will use invalid stack memory. -- v4: services: Cancel overlapped IO after timeout. https://gitlab.winehq.org/wine/wine/-/merge_requests/10424
From: Yuxuan Shui <yshui@codeweavers.com> Though WaitForSingleObject timed out, the IO operation is still running in the background. After the function process_send_command returns, the OVERLAPPED object referenced by this IO operation becomes invalid. Later, when this IO finally completes, server_select will use invalid stack memory. --- programs/services/rpc.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/programs/services/rpc.c b/programs/services/rpc.c index 2f4d4a5c26c..dceee099360 100644 --- a/programs/services/rpc.c +++ b/programs/services/rpc.c @@ -1192,7 +1192,7 @@ static BOOL service_accepts_control(const struct service_entry *service, DWORD d static BOOL process_send_command(struct process_entry *process, const void *data, DWORD size, DWORD *result) { OVERLAPPED overlapped; - DWORD count, ret; + DWORD count, ret = 0; BOOL r; overlapped.Offset = 0; @@ -1205,17 +1205,22 @@ static BOOL process_send_command(struct process_entry *process, const void *data if (ret == WAIT_TIMEOUT) { WINE_ERR("sending command timed out\n"); + if (!CancelIoEx(process->control_pipe, &overlapped)) + WINE_ERR("Failed to cancel IO, %#lx\n", GetLastError()); *result = ERROR_SERVICE_REQUEST_TIMEOUT; - return FALSE; } - r = GetOverlappedResult(process->control_pipe, &overlapped, &count, FALSE); + r = GetOverlappedResult(process->control_pipe, &overlapped, &count, TRUE); } if (!r || count != size) { - WINE_ERR("service protocol error - failed to write pipe!\n"); - *result = (!r ? GetLastError() : ERROR_WRITE_FAULT); + if (ret != WAIT_TIMEOUT) + { + WINE_ERR("service protocol error - failed to write pipe!\n"); + *result = (!r ? GetLastError() : ERROR_WRITE_FAULT); + } return FALSE; } + ret = 0; r = ReadFile(process->control_pipe, result, sizeof *result, &count, &overlapped); if (!r && GetLastError() == ERROR_IO_PENDING) { @@ -1223,16 +1228,20 @@ static BOOL process_send_command(struct process_entry *process, const void *data if (ret == WAIT_TIMEOUT) { WINE_ERR("receiving command result timed out\n"); + if (!CancelIoEx(process->control_pipe, &overlapped)) + WINE_ERR("Failed to cancel IO, %#lx\n", GetLastError()); *result = ERROR_SERVICE_REQUEST_TIMEOUT; - return FALSE; } - r = GetOverlappedResult(process->control_pipe, &overlapped, &count, FALSE); + r = GetOverlappedResult(process->control_pipe, &overlapped, &count, TRUE); } if (!r || count != sizeof *result) { - WINE_ERR("service protocol error - failed to read pipe " - "r = %d count = %ld!\n", r, count); - *result = (!r ? GetLastError() : ERROR_READ_FAULT); + if (ret != WAIT_TIMEOUT) + { + WINE_ERR("service protocol error - failed to read pipe " + "r = %d count = %ld!\n", r, count); + *result = (!r ? GetLastError() : ERROR_READ_FAULT); + } return FALSE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10424
update: added cancellation to both read and write timeouts. tweaked logic a bit, i feel it's clearer this way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10424#note_144816
On Thu Aug 6 09:22:54 2026 +0000, Yuxuan Shui wrote:
update: added cancellation to both read and write timeouts. tweaked logic a bit, i feel it's clearer this way. ping @iamahuman. could you have a look again?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10424#note_148137
Jinoh Kang (@iamahuman) commented about programs/services/rpc.c:
if (ret == WAIT_TIMEOUT) { WINE_ERR("receiving command result timed out\n"); + if (!CancelIoEx(process->control_pipe, &overlapped)) + WINE_ERR("Failed to cancel IO, %#lx\n", GetLastError()); *result = ERROR_SERVICE_REQUEST_TIMEOUT; - return FALSE; } - r = GetOverlappedResult(process->control_pipe, &overlapped, &count, FALSE); + r = GetOverlappedResult(process->control_pipe, &overlapped, &count, TRUE); } if (!r || count != sizeof *result) + { + if (ret != WAIT_TIMEOUT)
I think a separate boolean flag would be better. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10424#note_148870
Jinoh Kang (@iamahuman) commented about programs/services/rpc.c:
if (ret == WAIT_TIMEOUT) { WINE_ERR("receiving command result timed out\n"); + if (!CancelIoEx(process->control_pipe, &overlapped)) + WINE_ERR("Failed to cancel IO, %#lx\n", GetLastError()); *result = ERROR_SERVICE_REQUEST_TIMEOUT;
This has a subtle data race, since `result` doubles as the read buffer here. If the I/O completes right before this statement, then we will overwrite whatever result we have read from the pipe. The `result` write shall be done after I/O completion. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10424#note_148871
I suppose the previous approach of testing for `ERROR_OPERATION_ABORTED` is cleaner. No need for `ret != WAIT_TIMEOUT` hack or a bool flag that way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10424#note_148872
participants (3)
-
Jinoh Kang (＠iamahuman) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)