[PATCH] winhttp: Process end of read data before sending callback in read_data().
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- The read completion callback may call WineHttpReadData(). Which may either run synchronously (if the data is available and recursion depth limit is not reached) or queued to thread pool. Fallout76 always proceeds with last WinHttpReadData of size 0 with recursion limit reached, and finished_reading() is racing between the (synchronous) call which has read the last portion of data and the queued WinHttpReadData called with zero size sometimes causing a crash in cache_connection which may be called with NULL connection in this case. dlls/winhttp/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2c064bb767b..a9c65d15fa8 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1850,6 +1850,7 @@ static DWORD read_data( struct request *request, void *buffer, DWORD size, DWORD done: TRACE( "retrieved %u bytes (%u/%u)\n", bytes_read, request->content_read, request->content_length ); + if (end_of_read_data( request )) finished_reading( request ); if (async) { if (!ret) send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, buffer, bytes_read ); @@ -1863,7 +1864,6 @@ done: } if (!ret && read) *read = bytes_read; - if (end_of_read_data( request )) finished_reading( request ); return ret; } -- 2.31.1
On Thu, 2021-10-14 at 18:17 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- The read completion callback may call WineHttpReadData(). Which may either run synchronously (if the data is available and recursion depth limit is not reached) or queued to thread pool. Fallout76 always proceeds with last WinHttpReadData of size 0 with recursion limit reached, and finished_reading() is racing between the (synchronous) call which has read the last portion of data and the queued WinHttpReadData called with zero size sometimes causing a crash in cache_connection which may be called with NULL connection in this case.
dlls/winhttp/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2c064bb767b..a9c65d15fa8 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1850,6 +1850,7 @@ static DWORD read_data( struct request *request, void *buffer, DWORD size, DWORD
done: TRACE( "retrieved %u bytes (%u/%u)\n", bytes_read, request->content_read, request->content_length ); + if (end_of_read_data( request )) finished_reading( request ); if (async) { if (!ret) send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, buffer, bytes_read ); @@ -1863,7 +1864,6 @@ done: }
if (!ret && read) *read = bytes_read; - if (end_of_read_data( request )) finished_reading( request ); return ret; }
This could use a test because it changes the order of notifications. When finished_reading() closes the connection WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION will now be sent before WINHTTP_CALLBACK_STATUS_READ_COMPLETE.
On 10/15/21 13:04, Hans Leidekker wrote:
On Thu, 2021-10-14 at 18:17 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- The read completion callback may call WineHttpReadData(). Which may either run synchronously (if the data is available and recursion depth limit is not reached) or queued to thread pool. Fallout76 always proceeds with last WinHttpReadData of size 0 with recursion limit reached, and finished_reading() is racing between the (synchronous) call which has read the last portion of data and the queued WinHttpReadData called with zero size sometimes causing a crash in cache_connection which may be called with NULL connection in this case.
dlls/winhttp/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2c064bb767b..a9c65d15fa8 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1850,6 +1850,7 @@ static DWORD read_data( struct request *request, void *buffer, DWORD size, DWORD
done: TRACE( "retrieved %u bytes (%u/%u)\n", bytes_read, request->content_read, request->content_length ); + if (end_of_read_data( request )) finished_reading( request ); if (async) { if (!ret) send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, buffer, bytes_read ); @@ -1863,7 +1864,6 @@ done: }
if (!ret && read) *read = bytes_read; - if (end_of_read_data( request )) finished_reading( request ); return ret; } This could use a test because it changes the order of notifications. When finished_reading() closes the connection WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION will now be sent before WINHTTP_CALLBACK_STATUS_READ_COMPLETE.
Yes, I thought of that but found there is already read_allow_close_test[] used from test_persistent_connection(). This test lists connection close notifications before WINHTTP_CALLBACK_STATUS_READ_COMPLETE (which seems to be inlined with my change), although these are marked with NF_ALLOW and test succeeds both with and without my changes. Do you think I should try to make the test more conclusive?
On Fri, 2021-10-15 at 13:10 +0300, Paul Gofman wrote:
On 10/15/21 13:04, Hans Leidekker wrote:
On Thu, 2021-10-14 at 18:17 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- The read completion callback may call WineHttpReadData(). Which may either run synchronously (if the data is available and recursion depth limit is not reached) or queued to thread pool. Fallout76 always proceeds with last WinHttpReadData of size 0 with recursion limit reached, and finished_reading() is racing between the (synchronous) call which has read the last portion of data and the queued WinHttpReadData called with zero size sometimes causing a crash in cache_connection which may be called with NULL connection in this case.
dlls/winhttp/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2c064bb767b..a9c65d15fa8 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1850,6 +1850,7 @@ static DWORD read_data( struct request *request, void *buffer, DWORD size, DWORD
done: TRACE( "retrieved %u bytes (%u/%u)\n", bytes_read, request->content_read, request->content_length ); + if (end_of_read_data( request )) finished_reading( request ); if (async) { if (!ret) send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, buffer, bytes_read ); @@ -1863,7 +1864,6 @@ done: }
if (!ret && read) *read = bytes_read; - if (end_of_read_data( request )) finished_reading( request ); return ret; } This could use a test because it changes the order of notifications. When finished_reading() closes the connection WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION will now be sent before WINHTTP_CALLBACK_STATUS_READ_COMPLETE.
Yes, I thought of that but found there is already read_allow_close_test[] used from test_persistent_connection(). This test lists connection close notifications before WINHTTP_CALLBACK_STATUS_READ_COMPLETE (which seems to be inlined with my change), although these are marked with NF_ALLOW and test succeeds both with and without my changes. Do you think I should try to make the test more conclusive?
read_allow_close_test[] is unused since read_request_data() is always called with closing_connection set to FALSE. I'd suggest to rename it to read_close_test[] and require the exact notification sequence, if that's what native does.
participants (2)
-
Hans Leidekker -
Paul Gofman