Skip to content

Commit

Permalink
Resolve double free when WinHttpSendRequest fails (#1022)
Browse files Browse the repository at this point in the history 
* Update vcpkg.

* Resolve double free when WinHttpSendRequest fails

A customer reported that win WinHttpSendRequest fails, WinHTTP still
delivers WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING callbacks. When
_start_request_send encountered a failure, it deleted the context
weak_ptr, and then the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING
also tried to delete it, resulting in double frees.

This change binds the weak_ptr to the handle more directly and avoids paying attention to WinHttpSendRequest.
  • Loading branch information
@BillyONeal
BillyONeal authored Jan 19, 2019
1 parent e6498b2 commit c66f84d
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 60 deletions.
99 changes: 40 additions & 59 deletions Release/src/http/client/http_client_winhttp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ class winhttp_request_context final : public request_context
bool is_externally_allocated() const { return !m_body_data.is_internally_allocated(); }

HINTERNET m_request_handle;
std::weak_ptr<winhttp_request_context>* m_request_context;
std::weak_ptr<winhttp_request_context>*
m_request_handle_context; // owned by m_request_handle to be delete'd by WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING

bool m_proxy_authentication_tried;
bool m_server_authentication_tried;
Expand Down Expand Up @@ -532,9 +533,7 @@ class winhttp_request_context final : public request_context

if (m_request_handle != nullptr)
{
auto tmp_handle = m_request_handle;
m_request_handle = nullptr;
WinHttpCloseHandle(tmp_handle);
WinHttpCloseHandle(m_request_handle);
}
}

Expand Down Expand Up @@ -660,7 +659,6 @@ class winhttp_request_context final : public request_context
winhttp_request_context(const std::shared_ptr<_http_client_communicator>& client, const http_request& request)
: request_context(client, request)
, m_request_handle(nullptr)
, m_request_context(nullptr)
, m_bodyType(no_body)
, m_startingPosition(std::char_traits<uint8_t>::eof())
, m_body_data()
Expand Down Expand Up @@ -1004,6 +1002,8 @@ class winhttp_client final : public _http_client_communicator
http::uri_builder(m_uri).append(msg.relative_uri()).to_uri().resource().to_string();

// Open the request.
winhttp_context->m_request_handle_context = new std::weak_ptr<winhttp_request_context>(winhttp_context);

winhttp_context->m_request_handle =
WinHttpOpenRequest(m_hConnection,
msg.method().c_str(),
Expand All @@ -1015,10 +1015,24 @@ class winhttp_client final : public _http_client_communicator
if (winhttp_context->m_request_handle == nullptr)
{
auto errorCode = GetLastError();
delete winhttp_context->m_request_handle_context;
winhttp_context->m_request_handle_context = 0;
request->report_error(errorCode, build_error_msg(errorCode, "WinHttpOpenRequest"));
return;
}

if (!WinHttpSetOption(winhttp_context->m_request_handle,
WINHTTP_OPTION_CONTEXT_VALUE,
&winhttp_context->m_request_handle_context,
sizeof(void*)))
{
auto errorCode = GetLastError();
delete winhttp_context->m_request_handle_context;
winhttp_context->m_request_handle_context = 0;
request->report_error(errorCode, build_error_msg(errorCode, "WinHttpSetOption request context"));
return;
}

if (proxy_info_required)
{
auto result = WinHttpSetOption(
Expand Down Expand Up @@ -1194,69 +1208,36 @@ class winhttp_client final : public _http_client_communicator
private:
void _start_request_send(const std::shared_ptr<winhttp_request_context>& winhttp_context, size_t content_length)
{
// WinHttp takes a context object as a void*. We therefore heap allocate a std::weak_ptr to the request context
// which will be destroyed during the final callback.
std::unique_ptr<std::weak_ptr<winhttp_request_context>> weak_context_holder;
if (winhttp_context->m_request_context == nullptr)
DWORD totalLength;
if (winhttp_context->m_bodyType == no_body)
{
weak_context_holder = std::make_unique<std::weak_ptr<winhttp_request_context>>(winhttp_context);
winhttp_context->m_request_context = weak_context_holder.get();
totalLength = 0;
}

if (winhttp_context->m_bodyType == no_body)
else
{
if (!WinHttpSendRequest(winhttp_context->m_request_handle,
WINHTTP_NO_ADDITIONAL_HEADERS,
0,
nullptr,
0,
0,
(DWORD_PTR)winhttp_context->m_request_context))
{
if (weak_context_holder) winhttp_context->m_request_context = nullptr;
// Capture the current read position of the stream.
auto rbuf = winhttp_context->_get_readbuffer();

auto errorCode = GetLastError();
winhttp_context->report_error(errorCode, build_error_msg(errorCode, "WinHttpSendRequest"));
}
else
{
// Ownership of the weak_context_holder was accepted by the callback, so release the pointer without
// freeing.
weak_context_holder.release();
}
// Record starting position in case request is challenged for authorization
// and needs to seek back to where reading is started from.
winhttp_context->m_startingPosition = rbuf.getpos(std::ios_base::in);

return;
// If we find ourselves here, we either don't know how large the message
totalLength = winhttp_context->m_bodyType == content_length_chunked ? (DWORD)content_length
: WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH;
}

// Capture the current read position of the stream.
auto rbuf = winhttp_context->_get_readbuffer();

// Record starting position in case request is challenged for authorization
// and needs to seek back to where reading is started from.
winhttp_context->m_startingPosition = rbuf.getpos(std::ios_base::in);

// If we find ourselves here, we either don't know how large the message
// body is, or it is larger than our threshold.
if (!WinHttpSendRequest(winhttp_context->m_request_handle,
WINHTTP_NO_ADDITIONAL_HEADERS,
0,
nullptr,
0,
winhttp_context->m_bodyType == content_length_chunked
? (DWORD)content_length
: WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH,
(DWORD_PTR)winhttp_context->m_request_context))
const auto requestSuccess = WinHttpSendRequest(winhttp_context->m_request_handle,
WINHTTP_NO_ADDITIONAL_HEADERS,
0,
nullptr,
0,
totalLength,
(DWORD_PTR)winhttp_context->m_request_handle_context);
if (!requestSuccess)
{
if (weak_context_holder) winhttp_context->m_request_context = nullptr;

auto errorCode = GetLastError();
winhttp_context->report_error(errorCode, build_error_msg(errorCode, "WinHttpSendRequest chunked"));
}
else
{
// Ownership of the weak_context_holder was accepted by the callback, so release the pointer without
// freeing.
weak_context_holder.release();
winhttp_context->report_error(errorCode, build_error_msg(errorCode, "WinHttpSendRequest"));
}
}

Expand Down
2 changes: 1 addition & 1 deletion vcpkg
Submodule vcpkg updated 331 files

0 comments on commit c66f84d

Please sign in to comment.