Fixes in WinHttp WebSocket stack (#726)
This PR fixes a few different bugs in the WinHttp WebSocket stack: 1) The most severe bug is related to situations where WinHttpSendRequest fails synchronously. This can happen for a variety of reasons, though it does not seem to be consistent. According to msdn docs, (https://learn.microsoft.com/en-us/windows/win32/api/winhttp/nf-winhttp-winhttpsendrequest) "even when WinHTTP is used in asynchronous mode, that is, when WINHTTP_FLAG_ASYNC has been set in [WinHttpOpen](https://learn.microsoft.com/en-us/windows/desktop/api/winhttp/nf-winhttp-winhttpopen), this function can operate either synchronously or asynchronously." In our code, if the WinHttpSendRequest call failed synchronously, we'd clean up the WinHttp callback context which we still expected to exist to handle other WinHttp events (i.e. WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING) resulting in us accessing an invalid pointer. The fix is to release ownership of the callback context after WinHttpSetStatusCallback (rather than after WinHttpSendRequest). 2) The second bug that was in some error paths of HCWebSocketConnectAsync, the WinHttpCloseHandle never happens, nor is our WinHttp callback context ever cleaned up. The fix is to call StartWinHttpClose from WinHttpConnection::complete_task if the WebSocket Connect has fails. In cases where it succeeds, cleanup works correctly but doesn't take place until the WebSocket is later disconnected. This bug doesn't result in any crash, but does leak WinHttp handles and associated LHC context in some cases. 3) The final bug is that in some cases the client's WebSocket disconnect handler may be invoked multiple times. In some cases WinHttp can raise multiple error events (WINHTTP_CALLBACK_STATUS_REQUEST_ERROR), but we should only notify the client of a disconnect once. The fix is to add a flag to track whether we've already called the disconnect handler, and ignore subsequent WinHttp errors. This also doesn't cause any crashes, but could cause issues depending on how clients handle the disconnect event being fired multiple times.
This commit is contained in:
John L
GitHub
Родитель
7f502aa60d
Коммит
41e4e73cc1
|
|
@ -43,6 +43,12 @@ WinHttpConnection::~WinHttpConnection()
|
|||
{
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, __FUNCTION__);
|
||||
|
||||
// We should only ever be destroyed in the Initialized state or Closed state
|
||||
if (m_state != ConnectionState::Initialized && m_state != ConnectionState::Closed)
|
||||
{
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection destroyed in unexpected state (%lu)", m_state);
|
||||
}
|
||||
|
||||
if (m_state == ConnectionState::WebSocketConnected && m_hRequest && m_winHttpWebSocketExports.close)
|
||||
{
|
||||
// Use WinHttpWebSocketClose rather than disconnect in this case to close both the send and receive channels
|
||||
|
|
@ -291,7 +297,8 @@ HRESULT WinHttpConnection::HttpCallPerformAsync(XAsyncBlock* async)
|
|||
{
|
||||
RETURN_HR_IF(E_INVALIDARG, !async);
|
||||
m_asyncBlock = async;
|
||||
return SendRequest();
|
||||
SendRequest();
|
||||
return S_OK;
|
||||
}
|
||||
|
||||
#if !HC_NOWEBSOCKETS
|
||||
|
|
@ -395,6 +402,7 @@ HRESULT WinHttpConnection::Close(ConnectionClosedCallback callback)
|
|||
case ConnectionState::WebSocketConnected:
|
||||
{
|
||||
doWebSocketClose = true;
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::Close: ConnectionState=WebSocketConnected, Closing WebSocket");
|
||||
m_state = ConnectionState::WebSocketClosing;
|
||||
break;
|
||||
}
|
||||
|
|
@ -429,7 +437,7 @@ HRESULT WinHttpConnection::Close(ConnectionClosedCallback callback)
|
|||
}
|
||||
else if (doWinHttpClose)
|
||||
{
|
||||
return StartWinHttpClose();
|
||||
StartWinHttpClose();
|
||||
}
|
||||
else if (closeComplete)
|
||||
{
|
||||
|
|
@ -444,6 +452,9 @@ void WinHttpConnection::complete_task(_In_ HRESULT translatedHR)
|
|||
complete_task(translatedHR, translatedHR);
|
||||
}
|
||||
|
||||
// complete_task is used to complete the XAsync operation associated with either the HttpCallPerformAsync call (for pure HTTP WinHttpConnections)
|
||||
// or the WebSocketConnectAsync call (for WebSocket WinHttpConnections). In either case, XAsyncComplete should be called
|
||||
// exactly once. We reset m_asyncBlock after completing it so it doesn't get completed again.
|
||||
void WinHttpConnection::complete_task(_In_ HRESULT translatedHR, uint32_t platformSpecificError)
|
||||
{
|
||||
if (m_asyncBlock != nullptr)
|
||||
|
|
@ -461,8 +472,17 @@ void WinHttpConnection::complete_task(_In_ HRESULT translatedHR, uint32_t platfo
|
|||
XAsyncComplete(m_asyncBlock, S_OK, resultSize);
|
||||
m_asyncBlock = nullptr;
|
||||
}
|
||||
else
|
||||
{
|
||||
// This case can happen if WinHttp reports multiple errors for the same request. We will just complete the operation with the first
|
||||
// reported error and ignore subsequent ones
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::complete_task unexpectedly called multiple times. Ignoring subsequent call.");
|
||||
}
|
||||
|
||||
if (!m_websocketHandle)
|
||||
// If this is an HTTP request, we can always start WinHttp cleanup at this point.
|
||||
// If this is a WebSocket connection, we can start cleanup only if the connect failed. If the connect succeeded,
|
||||
// WinHttp cleanup won't happen until the WebSocket is disconnected.
|
||||
if (!m_websocketHandle || FAILED(translatedHR))
|
||||
{
|
||||
StartWinHttpClose();
|
||||
}
|
||||
|
|
@ -669,26 +689,39 @@ void WinHttpConnection::callback_status_request_error(
|
|||
}
|
||||
}
|
||||
|
||||
// Reissue the send if we were able to address a Cert error. All other errors are considered fatal.
|
||||
// Handle appropriately depending on if this is a WebSocket or Http request
|
||||
if (reissueSend)
|
||||
{
|
||||
HRESULT hr = pRequestContext->SendRequest();
|
||||
if (FAILED(hr))
|
||||
pRequestContext->SendRequest();
|
||||
}
|
||||
else if (pRequestContext->m_websocketHandle)
|
||||
{
|
||||
// If the WebSocket was connected (or previously connected) the error is treated as a disconnection.
|
||||
// If it hasn't yet been invoked, the client's disconnect handler will be invoked with the error. If the WebSocket wasn't yet
|
||||
// connected, complete the connect operation with the error
|
||||
bool disconnect{ false };
|
||||
{
|
||||
HC_TRACE_ERROR(HTTPCLIENT, "WinHttpConnection Failure to send HTTP request 0x%0.8x", hr);
|
||||
pRequestContext->complete_task(E_FAIL, hr);
|
||||
win32_cs_autolock autoCriticalSection(&pRequestContext->m_lock);
|
||||
if (pRequestContext->m_state == ConnectionState::WebSocketConnected || pRequestContext->m_state == ConnectionState::WebSocketClosing)
|
||||
{
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::callback_status_request_error after WebSocket was connected, moving to disconnect flow.");
|
||||
disconnect = true;
|
||||
}
|
||||
}
|
||||
|
||||
if (disconnect)
|
||||
{
|
||||
pRequestContext->on_websocket_disconnected(static_cast<USHORT>(errorCode));
|
||||
}
|
||||
else
|
||||
{
|
||||
pRequestContext->complete_task(E_FAIL, HRESULT_FROM_WIN32(errorCode));
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
if (pRequestContext->m_websocketHandle && (pRequestContext->m_state == ConnectionState::WebSocketConnected || pRequestContext->m_state == ConnectionState::WebSocketClosing))
|
||||
{
|
||||
// Only trigger if we're already connected, never during a connection attempt
|
||||
if (pRequestContext->m_asyncBlock == nullptr)
|
||||
{
|
||||
pRequestContext->on_websocket_disconnected(static_cast<USHORT>(errorCode));
|
||||
}
|
||||
}
|
||||
|
||||
// For Http requests, complete with error
|
||||
pRequestContext->complete_task(E_FAIL, HRESULT_FROM_WIN32(errorCode));
|
||||
}
|
||||
}
|
||||
|
|
@ -1094,6 +1127,13 @@ void CALLBACK WinHttpConnection::completion_callback(
|
|||
|
||||
case WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE:
|
||||
{
|
||||
// WinHttpWebSocketClose completion callback. Send and receive channells fully closed - no further messages can be sent or received.
|
||||
// Fire WebSocket disconnected event at this point.
|
||||
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE", TO_ULL(HCHttpCallGetId(pRequestContext->m_call)), GetCurrentThreadId());
|
||||
|
||||
// WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE opcode only associated with WebSocket connection, m_websocketHandle should never be null
|
||||
assert(pRequestContext->m_websocketHandle);
|
||||
if (pRequestContext->m_websocketHandle)
|
||||
{
|
||||
assert(pRequestContext->m_winHttpWebSocketExports.queryCloseStatus);
|
||||
|
|
@ -1107,6 +1147,18 @@ void CALLBACK WinHttpConnection::completion_callback(
|
|||
break;
|
||||
}
|
||||
|
||||
case WINHTTP_CALLBACK_STATUS_SHUTDOWN_COMPLETE:
|
||||
{
|
||||
// WinHttpWebSocketShutdown completion callback. Send channel closed, still expecting to receieve close frame from peer
|
||||
// and we will fire a disconnected event at that point.
|
||||
|
||||
// WINHTTP_CALLBACK_STATUS_SHUTDOWN_COMPLETE opcode only associated with WebSocket connection, m_websocketHandle should never be null
|
||||
assert(pRequestContext->m_websocketHandle);
|
||||
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WINHTTP_CALLBACK_STATUS_SHUTDOWN_COMPLETE", TO_ULL(HCHttpCallGetId(pRequestContext->m_call)), GetCurrentThreadId());
|
||||
break;
|
||||
}
|
||||
|
||||
case WINHTTP_CALLBACK_STATUS_SECURE_FAILURE:
|
||||
{
|
||||
callback_status_secure_failure(hRequestHandle, pRequestContext, statusInfo);
|
||||
|
|
@ -1115,10 +1167,12 @@ void CALLBACK WinHttpConnection::completion_callback(
|
|||
|
||||
case WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING:
|
||||
{
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING", TO_ULL(HCHttpCallGetId(pRequestContext->m_call)), GetCurrentThreadId());
|
||||
|
||||
// For WebSocket, we will also get a notification when the original request handle is closed. We have no action to take in that case
|
||||
if (hRequestHandle == pRequestContext->m_hRequest)
|
||||
{
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING", TO_ULL(HCHttpCallGetId(pRequestContext->m_call)), GetCurrentThreadId());
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection reclaiming WinHttpCallbackContext and invoking connectionClosedCallback");
|
||||
|
||||
// WinHttp Shutdown complete. WinHttp guarantees we will get no more callbacks for this request so we can safely cleanup context.
|
||||
// Ensure WinHttpCallbackContext is cleaned before invoking callback in case this is happening during HCCleanup
|
||||
|
|
@ -1205,7 +1259,7 @@ HRESULT WinHttpConnection::set_autodiscover_proxy()
|
|||
}
|
||||
#endif
|
||||
|
||||
HRESULT WinHttpConnection::SendRequest()
|
||||
void WinHttpConnection::SendRequest()
|
||||
{
|
||||
HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpConnection [%d] SendRequest", TO_ULL(HCHttpCallGetId(m_call)));
|
||||
|
||||
|
|
@ -1222,9 +1276,15 @@ HRESULT WinHttpConnection::SendRequest()
|
|||
{
|
||||
DWORD dwError = GetLastError();
|
||||
HC_TRACE_ERROR(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WinHttpSetStatusCallback errorcode %d", TO_ULL(HCHttpCallGetId(m_call)), GetCurrentThreadId(), dwError);
|
||||
return HRESULT_FROM_WIN32(dwError);
|
||||
|
||||
// Complete XAsync operation
|
||||
complete_task(E_FAIL, HRESULT_FROM_WIN32(dwError));
|
||||
return;
|
||||
}
|
||||
|
||||
// WinHttp callback successfully set. Error handling and cleanup path now go through completion_callback.
|
||||
// It is now responsible for cleaning up context when its no longer required. HC_UNIQUE_PTR is release with WinHttpSendRequest call
|
||||
|
||||
DWORD dwTotalLength = 0;
|
||||
switch (m_requestBodyType)
|
||||
{
|
||||
|
|
@ -1240,24 +1300,34 @@ HRESULT WinHttpConnection::SendRequest()
|
|||
nullptr,
|
||||
0,
|
||||
dwTotalLength,
|
||||
(DWORD_PTR)context.get()))
|
||||
(DWORD_PTR)context.release()))
|
||||
{
|
||||
DWORD dwError = GetLastError();
|
||||
HC_TRACE_ERROR(HTTPCLIENT, "WinHttpConnection [ID %llu] [TID %ul] WinHttpSendRequest errorcode %d", TO_ULL(HCHttpCallGetId(m_call)), GetCurrentThreadId(), dwError);
|
||||
return HRESULT_FROM_WIN32(dwError);
|
||||
|
||||
// Complete XAsync operation if WinHttpSendRequest fails synchronously
|
||||
complete_task(E_FAIL, HRESULT_FROM_WIN32(dwError));
|
||||
return;
|
||||
}
|
||||
|
||||
// WinHttp callback context will be reclaimed during WinHttp shutdown
|
||||
context.release();
|
||||
|
||||
return S_OK;
|
||||
// Flow continues from this point via WinHttp calling completion_callback
|
||||
}
|
||||
|
||||
HRESULT WinHttpConnection::StartWinHttpClose()
|
||||
void WinHttpConnection::StartWinHttpClose()
|
||||
{
|
||||
{
|
||||
win32_cs_autolock autoCriticalSection(&m_lock);
|
||||
m_state = ConnectionState::WinHttpClosing;
|
||||
|
||||
if (m_state == ConnectionState::WinHttpClosing || m_state == ConnectionState::Closed)
|
||||
{
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::StartWinHttpClose called while already closing, ignored");
|
||||
return;
|
||||
}
|
||||
else
|
||||
{
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::StartWinHttpClose, transitioning to ConnectionState::WinHttpClosing");
|
||||
m_state = ConnectionState::WinHttpClosing;
|
||||
}
|
||||
}
|
||||
|
||||
BOOL closed = WinHttpCloseHandle(m_hRequest);
|
||||
|
|
@ -1265,10 +1335,9 @@ HRESULT WinHttpConnection::StartWinHttpClose()
|
|||
{
|
||||
DWORD dwError = GetLastError();
|
||||
HC_TRACE_ERROR(HTTPCLIENT, "WinHttpCloseHandle failed with errorCode=%d", dwError);
|
||||
return HRESULT_FROM_WIN32(dwError);
|
||||
}
|
||||
|
||||
return S_OK;
|
||||
// Flow continues from this point via WinHttp calling completion_callback
|
||||
}
|
||||
|
||||
void WinHttpConnection::WebSocketSendMessage(const WebSocketSendContext& sendContext)
|
||||
|
|
@ -1311,6 +1380,17 @@ void WinHttpConnection::WebSocketCompleteEntireSendQueueWithError(HRESULT error)
|
|||
void WinHttpConnection::on_websocket_disconnected(_In_ USHORT closeReason)
|
||||
{
|
||||
#if !HC_NOWEBSOCKETS
|
||||
{
|
||||
win32_cs_autolock autoCriticalSection(&m_lock);
|
||||
// If we've already notified of disconnect, don't do it again
|
||||
if (m_disconnectHandlerInvoked)
|
||||
{
|
||||
HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpConnection::on_websocket_disconnected unexpectedly called multiple times. Ignoring subsequent call.");
|
||||
return;
|
||||
}
|
||||
m_disconnectHandlerInvoked = true;
|
||||
}
|
||||
|
||||
HCWebSocketCloseEventFunction disconnectFunc = nullptr;
|
||||
void* functionContext = nullptr;
|
||||
HCWebSocketGetEventFunctions(m_websocketHandle, nullptr, nullptr, &disconnectFunc, &functionContext);
|
||||
|
|
@ -1322,6 +1402,7 @@ void WinHttpConnection::on_websocket_disconnected(_In_ USHORT closeReason)
|
|||
}
|
||||
catch (...)
|
||||
{
|
||||
HC_TRACE_WARNING(HTTPCLIENT, "WinHttpConnection: Caught unhandled exception in client disconnect handler.");
|
||||
}
|
||||
|
||||
StartWinHttpClose();
|
||||
|
|
@ -1562,7 +1643,8 @@ HRESULT CALLBACK WinHttpConnection::WebSocketConnectProvider(XAsyncOp op, const
|
|||
{
|
||||
case XAsyncOp::Begin:
|
||||
{
|
||||
return winHttpConnection->SendRequest();
|
||||
winHttpConnection->SendRequest();
|
||||
return S_OK;
|
||||
}
|
||||
case XAsyncOp::GetResult:
|
||||
{
|
||||
|
|
|
|||
|
|
@ -269,8 +269,8 @@ private:
|
|||
void complete_task(_In_ HRESULT translatedHR);
|
||||
void complete_task(_In_ HRESULT translatedHR, uint32_t platformSpecificError);
|
||||
|
||||
HRESULT SendRequest();
|
||||
HRESULT StartWinHttpClose();
|
||||
void SendRequest();
|
||||
void StartWinHttpClose();
|
||||
|
||||
#if HC_PLATFORM != HC_PLATFORM_GDK
|
||||
HRESULT set_autodiscover_proxy();
|
||||
|
|
@ -321,6 +321,7 @@ private:
|
|||
// WebSocket state
|
||||
HCWebsocketHandle m_websocketHandle{ nullptr };
|
||||
HCCallHandle m_websocketCall{ nullptr };
|
||||
bool m_disconnectHandlerInvoked{ false };
|
||||
std::recursive_mutex m_websocketSendMutex; // controls access to m_websocketSendQueue
|
||||
http_internal_queue<WebSocketSendContext*> m_websocketSendQueue{};
|
||||
websocket_message_buffer m_websocketReceiveBuffer;
|
||||
|
|
|
|||
|
|
@ -302,7 +302,11 @@ HRESULT WinHttpProvider::CloseAllConnections()
|
|||
{
|
||||
for (auto& connection : connections)
|
||||
{
|
||||
connection->Close(connectionClosedCallback);
|
||||
HRESULT hr = connection->Close(connectionClosedCallback);
|
||||
if (FAILED(hr))
|
||||
{
|
||||
HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "WinHttpConnection::Close failed");
|
||||
}
|
||||
}
|
||||
WaitForSingleObject(closeContext.connectionsClosedEvent, INFINITE);
|
||||
}
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче