[PATCH] winhttp: Process end of read data before sending callback in read_data().

[Hans Leidekker]

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 at 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.