From 369f86ce6db1d20a62dd73a5223432e412af2100 Mon Sep 17 00:00:00 2001
From: Rick Winter <rick.winter@microsoft.com>
Date: Fri, 7 Apr 2023 15:52:58 -0700
Subject: [PATCH] [Azure.Core] Disable TCP_FAST_OPEN to avoid a WinHttp issue
 (#4530)

* Disable TCP_FAST_OPEN to avoid a WinHttp issue

* Add Changelog entry
---
 sdk/core/azure-core/CHANGELOG.md              |  1 +
 .../src/http/winhttp/win_http_transport.cpp   | 30 +++++++++++--------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md
index eb3cd30e0..dee973568 100644
--- a/sdk/core/azure-core/CHANGELOG.md
+++ b/sdk/core/azure-core/CHANGELOG.md
@@ -35,6 +35,7 @@ Thank you to our developer community members who helped to make Azure Core bette
 
 - [[#4213]](https://github.com/Azure/azure-sdk-for-cpp/issues/4213) Fixed a bug where `Host` request header is not set for non-default port (80, 443).
 - [[#4443]](https://github.com/Azure/azure-sdk-for-cpp/issues/4443) Fixed potentially high CPU usage on Windows.
+- [[#4490]](https://github.com/Azure/azure-sdk-for-cpp/issues/4490) Fixed WinHTTP memory leak during failed requests.
 
 ### Other Changes
 
diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp
index 26481b88d..f0276fddc 100644
--- a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp
+++ b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp
@@ -554,10 +554,10 @@ namespace Azure { namespace Core { namespace Http { namespace _detail {
    * multiple times based on the state of the TLS connection.
    *
    * Special consideration for the WINHTTP_CALLBACK_STATUS_SENDING_REQUEST - this callback is
-   * called during the TLS connection - if a TLS root certificate is configured, we verify that the
-   * certificate chain sent from the server contains the certificate the HTTP client was configured
-   * with. If it is, we accept the connection, if it is not, we abort the connection, closing the
-   * incoming request handle.
+   * called during the TLS connection - if a TLS root certificate is configured, we verify that
+   * the certificate chain sent from the server contains the certificate the HTTP client was
+   * configured with. If it is, we accept the connection, if it is not, we abort the connection,
+   * closing the incoming request handle.
    */
   void WinHttpAction::OnHttpStatusOperation(
       HINTERNET hInternet,
@@ -584,8 +584,8 @@ namespace Azure { namespace Core { namespace Http { namespace _detail {
     }
     else if (internetStatus == WINHTTP_CALLBACK_STATUS_SENDING_REQUEST)
     {
-      // We will only set the Status callback if a root certificate has been set. There is no action
-      // which needs to be completed for this notification.
+      // We will only set the Status callback if a root certificate has been set. There is no
+      // action which needs to be completed for this notification.
       m_httpRequest->HandleExpectedTlsRootCertificates(hInternet);
     }
     else if (internetStatus == m_expectedStatus)
@@ -605,8 +605,8 @@ namespace Azure { namespace Core { namespace Http { namespace _detail {
           CompleteAction();
           break;
         case WINHTTP_CALLBACK_STATUS_READ_COMPLETE:
-          // A WinHttpReadData call has completed. Complete the current action, including the amount
-          // of data read.
+          // A WinHttpReadData call has completed. Complete the current action, including the
+          // amount of data read.
           CompleteActionWithData(statusInformationLength);
           break;
         case WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING:
@@ -700,11 +700,15 @@ Azure::Core::_internal::UniqueHandle<HINTERNET> WinHttpTransport::CreateSessionH
     GetErrorAndThrow("Error while getting a session handle.");
   }
 
-// These options are only available starting from Windows 10 Version 2004, starting 06/09/2020.
-// These are primarily round trip time (RTT) performance optimizations, and hence if they don't get
-// set successfully, we shouldn't fail the request and continue as if the options don't exist.
-// Therefore, we just ignore the error and move on.
-#ifdef WINHTTP_OPTION_TCP_FAST_OPEN
+  // These options are only available starting from Windows 10 Version 2004, starting 06/09/2020.
+  // These are primarily round trip time (RTT) performance optimizations, and hence if they don't
+  // get set successfully, we shouldn't fail the request and continue as if the options don't exist.
+  // Therefore, we just ignore the error and move on.
+
+  // TCP_FAST_OPEN has a bug when the DNS resolution fails which can result
+  // in a leak.  Until that issue is fixed we've disable this option.
+
+#if defined(WINHTTP_OPTION_TCP_FAST_OPEN) && FALSE
   BOOL tcp_fast_open = TRUE;
   WinHttpSetOption(
       sessionHandle.get(), WINHTTP_OPTION_TCP_FAST_OPEN, &tcp_fast_open, sizeof(tcp_fast_open));