Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve double free when WinHttpSendRequest fails #1022

Merged
merged 4 commits into from
Jan 19, 2019
Merged

Resolve double free when WinHttpSendRequest fails #1022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0bd27b
Update vcpkg.
BillyONeal Jan 17, 2019
6302dd7
Resolve double free when WinHttpSendRequest fails
BillyONeal Jan 18, 2019
c4d4e08
Merge remote-tracking branch 'origin/master' into doublefree
BillyONeal Jan 18, 2019
ba4a5b8
Bind the request allocation to the handle more strongly.
BillyONeal Jan 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 40 additions & 59 deletions Release/src/http/client/http_client_winhttp.cpp
View file
Open in desktop
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;
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
Submodule vcpkg updated 331 files