Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit ce4ebdf

Browse files
davidshdavidsh
authored
Fix WinHttpHandler error handling (#28367)
This PR addresses some error handling problems in WinHttpHandler as well as improves the error messages generated for WinHttpException. This was root caused to a bug in WinHttp where it can't do a GET request with a request body using chunked encoding. However, this exception was masked due to an error handling bug caused by an inconsistency in how WinHttp associates context values for callbacks during the WinHttpSendRequest API call. This PR now fixes the error handling around synchronous errors being returned from WinHttpSendRequest. The root WinHttp bug itself can't be fixed. So, doing a GET request will still throw an exception, but it will be a catchable HttpRequestException. Another bug was discovered while investigating #26278. WinHttpCloseHandle should only be called once and should never race between threads. This is normally protected when the request handle is closed via SafeHandle Dispose(). But there was a place in the callback method where we called WinHttpCloseHandle directly against the raw handle. Finally, the error messages for WinHttpExceptions have been improved to show the error number and the probable WinHttp API call that generated the error. This will save diagnostic time. I also moved the WinHttpException source code back from Common. It was originally shared between WinHttpHandler and ClientWebSocket. But then ClientWebSocket moved away from WinHttp implementation. So, it makes more sense for this class to be under WinHttpHandler. Example: Original WinHttpException message: "The parameter is incorrect" New WinHttpException message: "Error 87 calling WinHttpSendRequest, 'The parameter is incorrect'." Closes #28156 Contributes to #26278
1 parent 08fd68b commit ce4ebdf

File tree

15 files changed

+102
-57
lines changed

15 files changed

+102
-57
lines changed

‎src/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@
121121
<data name="net_http_content_stream_already_read" xml:space="preserve">
122122
<value>The stream was already consumed. It cannot be read again.</value>
123123
</data>
124+
<data name="net_http_winhttp_error" xml:space="preserve">
125+
<value>Error {0} calling {1}, '{2}'.</value>
126+
</data>
124127
<data name="PlatformNotSupported_WinHttpHandler" xml:space="preserve">
125128
<value>WinHttpHandler is only supported on .NET Framework and .NET Core runtimes on Windows. It is not supported for Windows Store Applications (UWP) or Unix platforms.</value>
126129
</data>

‎src/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
<CompileItem Include="$(CommonPath)\System\Net\UriScheme.cs" />
2424
<CompileItem Include="$(CommonPath)\System\Net\Http\HttpHandlerDefaults.cs" />
2525
<CompileItem Include="$(CommonPath)\System\Net\Http\NoWriteNoSeekStreamContent.cs" />
26-
<CompileItem Include="$(CommonPath)\System\Net\Http\WinHttpException.cs" />
2726
<CompileItem Include="$(CommonPath)\System\Net\Security\CertificateHelper.cs" />
2827
<CompileItem Include="$(CommonPath)\System\Net\Security\CertificateHelper.Windows.cs" />
2928
<CompileItem Include="$(CommonPath)\System\Runtime\ExceptionServices\ExceptionStackTrace.cs" />
@@ -33,6 +32,7 @@
3332
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpCertificateHelper.cs" />
3433
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpChannelBinding.cs" />
3534
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpCookieContainerAdapter.cs" />
35+
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpException.cs" />
3636
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpHandler.cs" />
3737
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpRequestCallback.cs" />
3838
<CompileItem Include="$(MSBuildThisFileDirectory)\System\Net\Http\WinHttpRequestState.cs" />

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpAuthHelper.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ public void ChangeDefaultCredentialsPolicy(
296296
Interop.WinHttp.WINHTTP_OPTION_AUTOLOGON_POLICY,
297297
ref optionData))
298298
{
299-
WinHttpException.ThrowExceptionUsingLastError();
299+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
300300
}
301301
}
302302

@@ -365,7 +365,7 @@ private bool SetWinHttpCredential(
365365
password,
366366
IntPtr.Zero))
367367
{
368-
WinHttpException.ThrowExceptionUsingLastError();
368+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetCredentials));
369369
}
370370

371371
return true;

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCookieContainerAdapter.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public static void ResetCookieRequestHeaders(WinHttpRequestState state, Uri redi
6161
int lastError = Marshal.GetLastWin32Error();
6262
if (lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND)
6363
{
64-
throw WinHttpException.CreateExceptionUsingError(lastError);
64+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
6565
}
6666
}
6767

@@ -76,7 +76,7 @@ public static void ResetCookieRequestHeaders(WinHttpRequestState state, Uri redi
7676
(uint)cookieHeader.Length,
7777
Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
7878
{
79-
WinHttpException.ThrowExceptionUsingLastError();
79+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
8080
}
8181
}
8282
}

src/Common/src/System/Net/Http/WinHttpException.cs renamed to src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpException.cs

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System.ComponentModel;
6+
using System.Diagnostics;
67
using System.Runtime.ExceptionServices;
78
using System.Runtime.InteropServices;
89

@@ -39,37 +40,41 @@ public static int ConvertErrorCodeToHR(int error)
3940
}
4041
}
4142

42-
public static void ThrowExceptionUsingLastError()
43+
public static void ThrowExceptionUsingLastError(string nameOfCalledFunction)
4344
{
44-
throw CreateExceptionUsingLastError();
45+
throw CreateExceptionUsingLastError(nameOfCalledFunction);
4546
}
4647

47-
public static WinHttpException CreateExceptionUsingLastError()
48+
public static WinHttpException CreateExceptionUsingLastError(string nameOfCalledFunction)
4849
{
4950
int lastError = Marshal.GetLastWin32Error();
50-
return CreateExceptionUsingError(lastError);
51+
return CreateExceptionUsingError(lastError, nameOfCalledFunction);
5152
}
5253

53-
public static WinHttpException CreateExceptionUsingError(int error)
54+
public static WinHttpException CreateExceptionUsingError(int error, string nameOfCalledFunction)
5455
{
55-
var e = new WinHttpException(error, GetErrorMessage(error));
56+
var e = new WinHttpException(error, GetErrorMessage(error, nameOfCalledFunction));
5657
ExceptionStackTrace.AddCurrentStack(e);
5758
return e;
5859
}
5960

60-
public static WinHttpException CreateExceptionUsingError(int error, Exception innerException)
61+
public static WinHttpException CreateExceptionUsingError(int error, string nameOfCalledFunction, Exception innerException)
6162
{
62-
var e = new WinHttpException(error, GetErrorMessage(error), innerException);
63+
var e = new WinHttpException(error, GetErrorMessage(error, nameOfCalledFunction), innerException);
6364
ExceptionStackTrace.AddCurrentStack(e);
6465
return e;
6566
}
6667

67-
public static string GetErrorMessage(int error)
68+
public static string GetErrorMessage(int error, string nameOfCalledFunction)
6869
{
70+
Debug.Assert(!string.IsNullOrEmpty(nameOfCalledFunction));
71+
6972
// Look up specific error message in WINHTTP.DLL since it is not listed in default system resources
7073
// and thus can't be found by default .Net interop.
7174
IntPtr moduleHandle = Interop.Kernel32.GetModuleHandle(Interop.Libraries.WinHttp);
72-
return Interop.Kernel32.GetMessage(moduleHandle, error);
75+
string httpError = Interop.Kernel32.GetMessage(moduleHandle, error);
76+
77+
return SR.Format(SR.net_http_winhttp_error, error, nameOfCalledFunction, httpError);
7378
}
7479
}
7580
}

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -661,7 +661,7 @@ private static void AddRequestHeaders(
661661
(uint)requestHeadersBuffer.Length,
662662
Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
663663
{
664-
WinHttpException.ThrowExceptionUsingLastError();
664+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
665665
}
666666
}
667667

@@ -728,7 +728,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
728728
WinHttpTraceHelper.Trace("WinHttpHandler.EnsureSessionHandleExists: error={0}", lastError);
729729
if (lastError != Interop.WinHttp.ERROR_INVALID_PARAMETER)
730730
{
731-
ThrowOnInvalidHandle(sessionHandle);
731+
ThrowOnInvalidHandle(sessionHandle, nameof(Interop.WinHttp.WinHttpOpen));
732732
}
733733

734734
// We must be running on a platform earlier than Win8.1/Win2K12R2 which doesn't support
@@ -741,7 +741,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
741741
_proxyHelper.ManualSettingsOnly ? _proxyHelper.Proxy : Interop.WinHttp.WINHTTP_NO_PROXY_NAME,
742742
_proxyHelper.ManualSettingsOnly ? _proxyHelper.ProxyBypass : Interop.WinHttp.WINHTTP_NO_PROXY_BYPASS,
743743
(int)Interop.WinHttp.WINHTTP_FLAG_ASYNC);
744-
ThrowOnInvalidHandle(sessionHandle);
744+
ThrowOnInvalidHandle(sessionHandle, nameof(Interop.WinHttp.WinHttpOpen));
745745
}
746746

747747
uint optionAssuredNonBlockingTrue = 1; // TRUE
@@ -757,7 +757,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state)
757757
int lastError = Marshal.GetLastWin32Error();
758758
if (lastError != Interop.WinHttp.ERROR_WINHTTP_INVALID_OPTION)
759759
{
760-
throw WinHttpException.CreateExceptionUsingError(lastError);
760+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpSetOption));
761761
}
762762
}
763763

@@ -788,7 +788,7 @@ private async void StartRequest(WinHttpRequestState state)
788788
state.RequestMessage.RequestUri.Host,
789789
(ushort)state.RequestMessage.RequestUri.Port,
790790
0);
791-
ThrowOnInvalidHandle(connectHandle);
791+
ThrowOnInvalidHandle(connectHandle, nameof(Interop.WinHttp.WinHttpConnect));
792792
connectHandle.SetParentHandle(_sessionHandle);
793793

794794
// Try to use the requested version if a known/supported version was explicitly requested.
@@ -821,7 +821,7 @@ private async void StartRequest(WinHttpRequestState state)
821821
Interop.WinHttp.WINHTTP_NO_REFERER,
822822
Interop.WinHttp.WINHTTP_DEFAULT_ACCEPT_TYPES,
823823
flags);
824-
ThrowOnInvalidHandle(state.RequestHandle);
824+
ThrowOnInvalidHandle(state.RequestHandle, nameof(Interop.WinHttp.WinHttpOpenRequest));
825825
state.RequestHandle.SetParentHandle(connectHandle);
826826

827827
// Set callback function.
@@ -957,7 +957,7 @@ private void SetSessionHandleTimeoutOptions(SafeWinHttpHandle sessionHandle)
957957
(int)_sendTimeout.TotalMilliseconds,
958958
(int)_receiveHeadersTimeout.TotalMilliseconds))
959959
{
960-
WinHttpException.ThrowExceptionUsingLastError();
960+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetTimeouts));
961961
}
962962
}
963963

@@ -1214,7 +1214,7 @@ private void SetWinHttpOption(SafeWinHttpHandle handle, uint option, ref uint op
12141214
option,
12151215
ref optionData))
12161216
{
1217-
WinHttpException.ThrowExceptionUsingLastError();
1217+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
12181218
}
12191219
}
12201220

@@ -1227,7 +1227,7 @@ private void SetWinHttpOption(SafeWinHttpHandle handle, uint option, string opti
12271227
optionData,
12281228
(uint)optionData.Length))
12291229
{
1230-
WinHttpException.ThrowExceptionUsingLastError();
1230+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
12311231
}
12321232
}
12331233

@@ -1244,7 +1244,7 @@ private static void SetWinHttpOption(
12441244
optionData,
12451245
optionSize))
12461246
{
1247-
WinHttpException.ThrowExceptionUsingLastError();
1247+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSetOption));
12481248
}
12491249
}
12501250

@@ -1314,18 +1314,18 @@ private void SetStatusCallback(
13141314
int lastError = Marshal.GetLastWin32Error();
13151315
if (lastError != Interop.WinHttp.ERROR_INVALID_HANDLE) // Ignore error if handle was already closed.
13161316
{
1317-
throw WinHttpException.CreateExceptionUsingError(lastError);
1317+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpSetStatusCallback));
13181318
}
13191319
}
13201320
}
13211321

1322-
private void ThrowOnInvalidHandle(SafeWinHttpHandle handle)
1322+
private void ThrowOnInvalidHandle(SafeWinHttpHandle handle, string nameOfCalledFunction)
13231323
{
13241324
if (handle.IsInvalid)
13251325
{
13261326
int lastError = Marshal.GetLastWin32Error();
13271327
WinHttpTraceHelper.Trace("WinHttpHandler.ThrowOnInvalidHandle: error={0}", lastError);
1328-
throw WinHttpException.CreateExceptionUsingError(lastError);
1328+
throw WinHttpException.CreateExceptionUsingError(lastError, nameOfCalledFunction);
13291329
}
13301330
}
13311331

@@ -1343,11 +1343,13 @@ private RendezvousAwaitable<int> InternalSendRequestAsync(WinHttpRequestState st
13431343
0,
13441344
state.ToIntPtr()))
13451345
{
1346-
// Dispose (which will unpin) the state object. Since this failed, WinHTTP won't associate
1347-
// our context value (state object) to the request handle. And thus we won't get HANDLE_CLOSING
1348-
// notifications which would normally cause the state object to be unpinned and disposed.
1346+
// WinHTTP doesn't always associate our context value (state object) to the request handle.
1347+
// And thus we might not get a HANDLE_CLOSING notification which would normally cause the
1348+
// state object to be unpinned and disposed. So, we manually dispose the request handle and
1349+
// state object here.
1350+
state.RequestHandle.Dispose();
13491351
state.Dispose();
1350-
WinHttpException.ThrowExceptionUsingLastError();
1352+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpSendRequest));
13511353
}
13521354
}
13531355

@@ -1371,7 +1373,7 @@ private RendezvousAwaitable<int> InternalReceiveResponseHeadersAsync(WinHttpRequ
13711373
{
13721374
if (!Interop.WinHttp.WinHttpReceiveResponse(state.RequestHandle, IntPtr.Zero))
13731375
{
1374-
throw WinHttpException.CreateExceptionUsingLastError();
1376+
throw WinHttpException.CreateExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpReceiveResponse));
13751377
}
13761378
}
13771379

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,19 @@ private static void RequestCallback(
110110
}
111111
catch (Exception ex)
112112
{
113-
Interop.WinHttp.WinHttpCloseHandle(handle);
114113
state.SavedException = ex;
114+
if (state.RequestHandle != null)
115+
{
116+
// Since we got a fatal error processing the request callback,
117+
// we need to close the WinHttp request handle in order to
118+
// abort the currently executing WinHttp async operation.
119+
//
120+
// We must always call Dispose() against the SafeWinHttpHandle
121+
// wrapper and never close directly the raw WinHttp handle.
122+
// The SafeWinHttpHandle wrapper is thread-safe and guarantees
123+
// calling the underlying WinHttpCloseHandle() function only once.
124+
state.RequestHandle.Dispose();
125+
}
115126
}
116127
}
117128

@@ -260,7 +271,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
260271
return;
261272
}
262273

263-
throw WinHttpException.CreateExceptionUsingError(lastError);
274+
throw WinHttpException.CreateExceptionUsingError(lastError, "WINHTTP_CALLBACK_STATUS_SENDING_REQUEST/WinHttpQueryOption");
264275
}
265276

266277
// Get any additional certificates sent from the remote server during the TLS/SSL handshake.
@@ -295,7 +306,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
295306
catch (Exception ex)
296307
{
297308
throw WinHttpException.CreateExceptionUsingError(
298-
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, ex);
309+
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, "X509Chain.Build", ex);
299310
}
300311
finally
301312
{
@@ -310,7 +321,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
310321
if (!result)
311322
{
312323
throw WinHttpException.CreateExceptionUsingError(
313-
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE);
324+
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, "ServerCertificateValidationCallback");
314325
}
315326
}
316327
}
@@ -321,7 +332,7 @@ private static void OnRequestError(WinHttpRequestState state, Interop.WinHttp.WI
321332

322333
Debug.Assert(state != null, "OnRequestError: state is null");
323334

324-
Exception innerException = WinHttpException.CreateExceptionUsingError(unchecked((int)asyncResult.dwError));
335+
Exception innerException = WinHttpException.CreateExceptionUsingError(unchecked((int)asyncResult.dwError), "WINHTTP_CALLBACK_STATUS_REQUEST_ERROR");
325336

326337
switch (unchecked((uint)asyncResult.dwResult.ToInt32()))
327338
{
@@ -424,7 +435,7 @@ private static void ResetAuthRequestHeaders(WinHttpRequestState state)
424435
int lastError = Marshal.GetLastWin32Error();
425436
if (lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND)
426437
{
427-
throw WinHttpException.CreateExceptionUsingError(lastError);
438+
throw WinHttpException.CreateExceptionUsingError(lastError, "WINHTTP_CALLBACK_STATUS_REDIRECT/WinHttpAddRequestHeaders");
428439
}
429440
}
430441
}

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ private Task<bool> InternalWriteDataAsync(byte[] buffer, int offset, int count,
249249
IntPtr.Zero))
250250
{
251251
_state.TcsInternalWriteDataToRequestStream.TrySetException(
252-
new IOException(SR.net_http_io_write, WinHttpException.CreateExceptionUsingLastError()));
252+
new IOException(SR.net_http_io_write, WinHttpException.CreateExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpWriteData))));
253253
}
254254
}
255255

‎src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public static uint GetResponseHeaderNumberInfo(SafeWinHttpHandle requestHandle,
127127
ref resultSize,
128128
IntPtr.Zero))
129129
{
130-
WinHttpException.ThrowExceptionUsingLastError();
130+
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpQueryHeaders));
131131
}
132132

133133
return result;
@@ -189,7 +189,7 @@ public static unsafe bool GetResponseHeader(
189189
return GetResponseHeader(requestHandle, infoLevel, ref buffer, ref index, out headerValue);
190190
}
191191

192-
throw WinHttpException.CreateExceptionUsingError(lastError);
192+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
193193
}
194194

195195
/// <summary>
@@ -216,7 +216,7 @@ private static unsafe int GetResponseHeader(SafeWinHttpHandle requestHandle, uin
216216

217217
Debug.Assert(lastError != Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER, "buffer must be of sufficient size.");
218218

219-
throw WinHttpException.CreateExceptionUsingError(lastError);
219+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
220220
}
221221
}
222222

@@ -240,7 +240,7 @@ private static unsafe int GetResponseHeaderCharBufferLength(SafeWinHttpHandle re
240240

241241
if (lastError != Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER)
242242
{
243-
throw WinHttpException.CreateExceptionUsingError(lastError);
243+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
244244
}
245245
}
246246

0 commit comments

Comments
 (0)