From d0bd27b820e17f10cf048c2fe7387b76e3254795 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Thu, 17 Jan 2019 15:32:48 -0800 Subject: [PATCH 1/3] Update vcpkg. --- vcpkg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vcpkg b/vcpkg index fcde2e64eb..c028db0b8d 160000 --- a/vcpkg +++ b/vcpkg @@ -1 +1 @@ -Subproject commit fcde2e64eb46295b64b3caa58d0d32eab0b6a49d +Subproject commit c028db0b8d8a60b60963c5648d96af4ae02050d5 From 6302dd762c2d071e9421fe8a533f3488154d64c8 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Thu, 17 Jan 2019 19:51:02 -0800 Subject: [PATCH 2/3] 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. We don't fully understand WinHTTP's behavior and it's possible this may leak a bit, but that's better than a double free. --- .../src/http/client/http_client_winhttp.cpp | 79 ++++++------------- 1 file changed, 24 insertions(+), 55 deletions(-) diff --git a/Release/src/http/client/http_client_winhttp.cpp b/Release/src/http/client/http_client_winhttp.cpp index 70cf9f7257..f28dea4b3c 100644 --- a/Release/src/http/client/http_client_winhttp.cpp +++ b/Release/src/http/client/http_client_winhttp.cpp @@ -244,7 +244,6 @@ 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* m_request_context; bool m_proxy_authentication_tried; bool m_server_authentication_tried; @@ -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::eof()) , m_body_data() @@ -1196,67 +1194,38 @@ class winhttp_client final : public _http_client_communicator { // 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> weak_context_holder; - if (winhttp_context->m_request_context == nullptr) - { - weak_context_holder = std::make_unique>(winhttp_context); - winhttp_context->m_request_context = weak_context_holder.get(); - } - - if (winhttp_context->m_bodyType == no_body) - { - 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; - - 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(); - } - - return; + std::unique_ptr> weak_context_holder + = std::make_unique>(winhttp_context); + + DWORD totalLength; + if (winhttp_context->m_bodyType == no_body) { + totalLength = 0; + } else { + // 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 + 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, + const auto requestSuccess = 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)) + totalLength, + (DWORD_PTR)weak_context_holder.get()); + weak_context_holder.release(); // to be freed in WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING + 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")); } } From ba4a5b84431990bd03d0cf7a3d4404e7ef3d5c79 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Fri, 18 Jan 2019 16:05:06 -0800 Subject: [PATCH 3/3] Bind the request allocation to the handle more strongly. --- .../src/http/client/http_client_winhttp.cpp | 52 ++++++++++++------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/Release/src/http/client/http_client_winhttp.cpp b/Release/src/http/client/http_client_winhttp.cpp index f28dea4b3c..eb94f0f984 100644 --- a/Release/src/http/client/http_client_winhttp.cpp +++ b/Release/src/http/client/http_client_winhttp.cpp @@ -244,6 +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* + 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; @@ -531,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); } } @@ -1002,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_context); + winhttp_context->m_request_handle = WinHttpOpenRequest(m_hConnection, msg.method().c_str(), @@ -1013,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( @@ -1192,15 +1208,13 @@ class winhttp_client final : public _http_client_communicator private: void _start_request_send(const std::shared_ptr& 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> weak_context_holder - = std::make_unique>(winhttp_context); - DWORD totalLength; - if (winhttp_context->m_bodyType == no_body) { + if (winhttp_context->m_bodyType == no_body) + { totalLength = 0; - } else { + } + else + { // Capture the current read position of the stream. auto rbuf = winhttp_context->_get_readbuffer(); @@ -1209,19 +1223,17 @@ class winhttp_client final : public _http_client_communicator winhttp_context->m_startingPosition = rbuf.getpos(std::ios_base::in); // 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; + totalLength = winhttp_context->m_bodyType == content_length_chunked ? (DWORD)content_length + : WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH; } const auto requestSuccess = WinHttpSendRequest(winhttp_context->m_request_handle, - WINHTTP_NO_ADDITIONAL_HEADERS, - 0, - nullptr, - 0, - totalLength, - (DWORD_PTR)weak_context_holder.get()); - weak_context_holder.release(); // to be freed in WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING + WINHTTP_NO_ADDITIONAL_HEADERS, + 0, + nullptr, + 0, + totalLength, + (DWORD_PTR)winhttp_context->m_request_handle_context); if (!requestSuccess) { auto errorCode = GetLastError();