Re: [PATCH] winhttp: Use lock in send_frame().

[Paul Gofman]

Eh, I am sorry, I realized that the patch probably doesn't really fully 
solve the described possible race between queued and immediate send. I 
will revise it and send another version. It should probably be locked 
before incrementing the counter.

On 2/2/22 11:53, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
>   While web socket sends are not actually supposed to be queued before the previous one completed, send for pongs
>   or socket shutdown may be called in parallel. Only the first call will go for synchronous send attempt but the
>   next queued call have a chance of hitting socket_send() before the first sync one returns.
>   The sync outside of send_frame() (which is needed if the send wasn't completed synchronously) is provided by
>   the queueing logic around pending_sends counter.
>
> dlls/winhttp/request.c         | 12 +++++++++---
>   dlls/winhttp/winhttp_private.h |  1 +
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
> index 728b1895ba7..756b40d8987 100644
> --- a/dlls/winhttp/request.c
> +++ b/dlls/winhttp/request.c
> @@ -3111,6 +3111,7 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR
>       socket->hdr.callback = request->hdr.callback;
>       socket->hdr.notify_mask = request->hdr.notify_mask;
>       socket->hdr.context = context;
> -
>
> InitializeSRWLock( &socket->send_lock );
>
> addref_object( &request->hdr );
>  socket->request = request;
>
> @@ -3153,6 +3154,8 @@ static DWORD send_frame( struct socket *socket, enum socket_opcode opcode, USHOR
>   TRACE( "sending %02x frame, len %u.\n", opcode, len );
>
> -
>
> AcquireSRWLockExclusive( &socket->send_lock );
> -
> if (opcode == SOCKET_OPCODE_CLOSE) len += sizeof(status);
>
> hdr[0] = final ? (char)FIN_BIT : 0;
>
> @@ -3188,7 +3191,8 @@ static DWORD send_frame( struct socket *socket, enum socket_opcode opcode, USHOR
>           if (!(new = realloc( socket->send_frame_buffer, new_size )))
>           {
>               ERR("Out of memory, buffer_size %u.\n", buffer_size);
> -
>        return ERROR_OUTOFMEMORY;
> -
>        ret = ERROR_OUTOFMEMORY;
> -
>        goto done;
>     }
>     socket->send_frame_buffer = new;
>     socket->send_frame_buffer_size = new_size;
>
> @@ -3231,13 +3235,15 @@ static DWORD send_frame( struct socket *socket, enum socket_opcode opcode, USHOR
>                   memmove( socket->send_frame_buffer, socket->send_frame_buffer + sent_size, offset - sent_size );
>                   socket->bytes_in_send_frame_buffer = offset - sent_size;
>               }
> -
>        return ret;
> -
>        goto done;
>     }
>     assert( sent_size == offset );
>     offset = 0;
>     buflen -= len;
> }
> - return ERROR_SUCCESS;
>
> +done:
> -
>
> ReleaseSRWLockExclusive( &socket->send_lock );
> -
>
> return ret;
> }
>
> static DWORD complete_send_frame( struct socket *socket, WSAOVERLAPPED *ovr, const char *buf )
>
> diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h
> index 3e4e1eb298d..732f0afaa88 100644
> --- a/dlls/winhttp/winhttp_private.h
> +++ b/dlls/winhttp/winhttp_private.h
> @@ -259,6 +259,7 @@ struct socket
>       unsigned int send_remaining_size;
>       unsigned int bytes_in_send_frame_buffer;
>       unsigned int client_buffer_offset;
> -
>
> SRWLOCK send_lock;
> };
>
> struct send_request