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

Commit 8611674

Browse files
CIPopCIPop
authored
[2.0 Port] Fixing NativeOverlapped lifetime issue (#21100)
* Fixing NativeOverlapped lifetime issue. Adding a test to validate. (#20793) * Fixing NativeOverlapped lifetime issue. Adding a test to validate. * Renaming DEBUG only methods. * Tracking managed implementation bug. * Renaming file to match classname. * Formatting changes * Addressing PR feedback.
1 parent f128caa commit 8611674

File tree

8 files changed

+125
-31
lines changed

8 files changed

+125
-31
lines changed

‎src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -605,17 +605,6 @@ override protected bool ReleaseHandle()
605605
[DllImport(Libraries.HttpApi, SetLastError = true)]
606606
internal static extern unsafe uint HttpReceiveClientCertificate(SafeHandle requestQueueHandle, ulong connectionId, uint flags, byte* pSslClientCertInfo, uint sslClientCertInfoSize, uint* pBytesReceived, NativeOverlapped* pOverlapped);
607607

608-
[Flags]
609-
internal enum FileCompletionNotificationModes : byte
610-
{
611-
None = 0,
612-
SkipCompletionPortOnSuccess = 1,
613-
SkipSetEventOnHandle = 2
614-
}
615-
616-
[DllImport(Libraries.Kernel32, SetLastError = true)]
617-
internal static extern unsafe bool SetFileCompletionNotificationModes(SafeHandle handle, FileCompletionNotificationModes modes);
618-
619608
internal static readonly string[] HttpVerbs = new string[]
620609
{
621610
null,

‎src/System.Net.HttpListener/src/System.Net.HttpListener.csproj‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,9 @@
187187
<Compile Include="$(CommonPath)\Interop\Windows\HttpApi\Interop.HttpApi.cs">
188188
<Link>Common\Interop\Windows\HttpApi\Interop.HttpApi.cs</Link>
189189
</Compile>
190+
<Compile Include="$(CommonPath)\Interop\Windows\kernel32\Interop.SetFileCompletionNotificationModes.cs">
191+
<Link>Interop\Windows\kernel32\Interop.SetFileCompletionNotificationModes.cs</Link>
192+
</Compile>
190193
<Compile Include="$(CommonPath)\Interop\Windows\kernel32\Interop.CancelIoEx.cs">
191194
<Link>Common\Interop\Windows\mincore\Interop.CancelIoEx.cs</Link>
192195
</Compile>

‎src/System.Net.HttpListener/src/System/Net/Windows/AsyncRequestContext.cs‎

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,22 @@ internal sealed unsafe class AsyncRequestContext : RequestContextBase
1414
private ThreadPoolBoundHandle _boundHandle;
1515
private readonly ListenerAsyncResult _result;
1616

17+
#if DEBUG
18+
private volatile int _nativeOverlappedCounter = 0;
19+
private volatile int _nativeOverlappedUsed = 0;
20+
21+
private void DebugRefCountReleaseNativeOverlapped()
22+
{
23+
Debug.Assert(Interlocked.Decrement(ref _nativeOverlappedCounter) == 0, "NativeOverlapped released too many times.");
24+
Interlocked.Decrement(ref _nativeOverlappedUsed);
25+
}
26+
27+
private void DebugRefCountAllocNativeOverlapped()
28+
{
29+
Debug.Assert(Interlocked.Increment(ref _nativeOverlappedCounter) == 1, "NativeOverlapped allocated without release.");
30+
}
31+
#endif
32+
1733
internal AsyncRequestContext(ThreadPoolBoundHandle boundHandle, ListenerAsyncResult result)
1834
{
1935
_result = result;
@@ -23,20 +39,26 @@ internal AsyncRequestContext(ThreadPoolBoundHandle boundHandle, ListenerAsyncRes
2339
private Interop.HttpApi.HTTP_REQUEST* Allocate(ThreadPoolBoundHandle boundHandle, uint size)
2440
{
2541
uint newSize = size != 0 ? size : RequestBuffer == null ? 4096 : Size;
26-
if (_nativeOverlapped != null && newSize != RequestBuffer.Length)
42+
if (_nativeOverlapped != null)
2743
{
44+
#if DEBUG
45+
DebugRefCountReleaseNativeOverlapped();
46+
#endif
47+
2848
NativeOverlapped* nativeOverlapped = _nativeOverlapped;
2949
_nativeOverlapped = null;
3050
_boundHandle.FreeNativeOverlapped(nativeOverlapped);
3151
}
32-
if (_nativeOverlapped == null)
33-
{
34-
SetBuffer(checked((int)newSize));
35-
_boundHandle = boundHandle;
36-
_nativeOverlapped = boundHandle.AllocateNativeOverlapped(ListenerAsyncResult.IOCallback, state: _result, pinData: RequestBuffer);
37-
return (Interop.HttpApi.HTTP_REQUEST*)Marshal.UnsafeAddrOfPinnedArrayElement(RequestBuffer, 0);
38-
}
39-
return RequestBlob;
52+
53+
Debug.Assert(_nativeOverlapped == null);
54+
#if DEBUG
55+
DebugRefCountAllocNativeOverlapped();
56+
#endif
57+
SetBuffer(checked((int)newSize));
58+
_boundHandle = boundHandle;
59+
_nativeOverlapped = boundHandle.AllocateNativeOverlapped(ListenerAsyncResult.IOCallback, state: _result, pinData: RequestBuffer);
60+
61+
return (Interop.HttpApi.HTTP_REQUEST*)Marshal.UnsafeAddrOfPinnedArrayElement(RequestBuffer, 0);
4062
}
4163

4264
internal void Reset(ThreadPoolBoundHandle boundHandle, ulong requestId, uint size)
@@ -49,6 +71,10 @@ protected override void OnReleasePins()
4971
{
5072
if (_nativeOverlapped != null)
5173
{
74+
#if DEBUG
75+
DebugRefCountReleaseNativeOverlapped();
76+
#endif
77+
5278
NativeOverlapped* nativeOverlapped = _nativeOverlapped;
5379
_nativeOverlapped = null;
5480
_boundHandle.FreeNativeOverlapped(nativeOverlapped);
@@ -62,19 +88,26 @@ protected override void Dispose(bool disposing)
6288
Debug.Assert(!disposing, "AsyncRequestContext::Dispose()|Must call ReleasePins() before calling Dispose().");
6389
if (!Environment.HasShutdownStarted || disposing)
6490
{
91+
#if DEBUG
92+
DebugRefCountReleaseNativeOverlapped();
93+
#endif
6594
_boundHandle.FreeNativeOverlapped(_nativeOverlapped);
6695
}
6796
}
97+
6898
base.Dispose(disposing);
6999
}
70100

71101
internal NativeOverlapped* NativeOverlapped
72102
{
73103
get
74104
{
105+
#if DEBUG
106+
Debug.Assert(Interlocked.Increment(ref _nativeOverlappedUsed) == 1, "NativeOverlapped reused.");
107+
#endif
108+
75109
return _nativeOverlapped;
76110
}
77111
}
78112
}
79113
}
80-

‎src/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,9 +177,11 @@ internal ThreadPoolBoundHandle RequestQueueBoundHandle
177177
if (_requestQueueBoundHandle == null)
178178
{
179179
_requestQueueBoundHandle = ThreadPoolBoundHandle.BindHandle(_requestQueueHandle);
180+
if (NetEventSource.IsEnabled) NetEventSource.Info($"ThreadPoolBoundHandle.BindHandle({_requestQueueHandle}) -> {_requestQueueBoundHandle}");
180181
}
181182
}
182183
}
184+
183185
return _requestQueueBoundHandle;
184186
}
185187
}
@@ -436,10 +438,10 @@ private unsafe void CreateRequestQueueHandle()
436438

437439
// Disabling callbacks when IO operation completes synchronously (returns ErrorCodes.ERROR_SUCCESS)
438440
if (SkipIOCPCallbackOnSuccess &&
439-
!Interop.HttpApi.SetFileCompletionNotificationModes(
441+
!Interop.Kernel32.SetFileCompletionNotificationModes(
440442
requestQueueHandle,
441-
Interop.HttpApi.FileCompletionNotificationModes.SkipCompletionPortOnSuccess |
442-
Interop.HttpApi.FileCompletionNotificationModes.SkipSetEventOnHandle))
443+
Interop.Kernel32.FileCompletionNotificationModes.SkipCompletionPortOnSuccess |
444+
Interop.Kernel32.FileCompletionNotificationModes.SkipSetEventOnHandle))
443445
{
444446
throw new HttpListenerException(Marshal.GetLastWin32Error());
445447
}
@@ -451,6 +453,7 @@ private unsafe void CloseRequestQueueHandle()
451453
{
452454
if ((_requestQueueHandle != null) && (!_requestQueueHandle.IsInvalid))
453455
{
456+
if (NetEventSource.IsEnabled) NetEventSource.Info($"Dispose ThreadPoolBoundHandle: {_requestQueueBoundHandle}");
454457
_requestQueueBoundHandle?.Dispose();
455458
_requestQueueHandle.Dispose();
456459
}
@@ -1914,8 +1917,10 @@ internal unsafe DisconnectAsyncResult(HttpListener httpListener, ulong connectio
19141917
_ownershipState = 1;
19151918
_httpListener = httpListener;
19161919
_connectionId = connectionId;
1920+
19171921
// we can call the Unsafe API here, we won't ever call user code
19181922
_nativeOverlapped = httpListener.RequestQueueBoundHandle.AllocateNativeOverlapped(s_IOCallback, state: this, pinData: null);
1923+
if (NetEventSource.IsEnabled) NetEventSource.Info($"DisconnectAsyncResult: ThreadPoolBoundHandle.AllocateNativeOverlapped({httpListener._requestQueueBoundHandle}) -> {_nativeOverlapped->GetHashCode()}");
19191924
}
19201925

19211926
internal bool StartOwningDisconnectHandling()
@@ -1950,6 +1955,7 @@ internal unsafe void IOCompleted(uint errorCode, uint numBytes, NativeOverlapped
19501955
private static unsafe void IOCompleted(DisconnectAsyncResult asyncResult, uint errorCode, uint numBytes, NativeOverlapped* nativeOverlapped)
19511956
{
19521957
if (NetEventSource.IsEnabled) NetEventSource.Info(null, "_connectionId:" + asyncResult._connectionId);
1958+
19531959
asyncResult._httpListener._requestQueueBoundHandle.FreeNativeOverlapped(nativeOverlapped);
19541960
if (Interlocked.Exchange(ref asyncResult._ownershipState, 2) == 0)
19551961
{

‎src/System.Net.HttpListener/src/System/Net/Windows/ListenerAsyncResult.Windows.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,7 @@ internal uint QueueBeginGetContext()
132132
_requestContext.Reset(listener.RequestQueueBoundHandle, _requestContext.RequestBlob->RequestId, bytesTransferred);
133133
continue;
134134
}
135-
else if (statusCode == Interop.HttpApi.ERROR_SUCCESS &&
136-
HttpListener.SkipIOCPCallbackOnSuccess)
135+
else if (statusCode == Interop.HttpApi.ERROR_SUCCESS && HttpListener.SkipIOCPCallbackOnSuccess)
137136
{
138137
// IO operation completed synchronously - callback won't be called to signal completion.
139138
IOCompleted(this, statusCode, bytesTransferred);

‎src/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketHttpListenerDuplexStream.cs‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -852,6 +852,22 @@ internal class HttpListenerAsyncEventArgs : EventArgs, IDisposable
852852
private readonly WebSocketBase _webSocket;
853853
private readonly WebSocketHttpListenerDuplexStream _currentStream;
854854

855+
#if DEBUG
856+
private volatile int _nativeOverlappedCounter = 0;
857+
private volatile int _nativeOverlappedUsed = 0;
858+
859+
private void DebugRefCountReleaseNativeOverlapped()
860+
{
861+
Debug.Assert(Interlocked.Decrement(ref _nativeOverlappedCounter) == 0, "NativeOverlapped released too many times.");
862+
Interlocked.Decrement(ref _nativeOverlappedUsed);
863+
}
864+
865+
private void DebugRefCountAllocNativeOverlapped()
866+
{
867+
Debug.Assert(Interlocked.Increment(ref _nativeOverlappedCounter) == 1, "NativeOverlapped allocated without release.");
868+
}
869+
#endif
870+
855871
public HttpListenerAsyncEventArgs(WebSocketBase webSocket, WebSocketHttpListenerDuplexStream stream)
856872
: base()
857873
{
@@ -923,7 +939,13 @@ public ushort EntityChunkCount
923939

924940
internal unsafe NativeOverlapped* NativeOverlapped
925941
{
926-
get { return _ptrNativeOverlapped; }
942+
get
943+
{
944+
#if DEBUG
945+
Debug.Assert(Interlocked.Increment(ref _nativeOverlappedUsed) == 1, "NativeOverlapped reused.");
946+
#endif
947+
return _ptrNativeOverlapped;
948+
}
927949
}
928950

929951
public IntPtr EntityChunks
@@ -986,6 +1008,9 @@ public void Dispose()
9861008

9871009
private unsafe void InitializeOverlapped(ThreadPoolBoundHandle boundHandle)
9881010
{
1011+
#if DEBUG
1012+
DebugRefCountAllocNativeOverlapped();
1013+
#endif
9891014
_boundHandle = boundHandle;
9901015
_ptrNativeOverlapped = boundHandle.AllocateNativeOverlapped(CompletionPortCallback, null, null);
9911016
}
@@ -998,6 +1023,9 @@ private unsafe void FreeOverlapped(bool checkForShutdown)
9981023
// Free the overlapped object
9991024
if (_ptrNativeOverlapped != null)
10001025
{
1026+
#if DEBUG
1027+
DebugRefCountReleaseNativeOverlapped();
1028+
#endif
10011029
_boundHandle.FreeNativeOverlapped(_ptrNativeOverlapped);
10021030
_ptrNativeOverlapped = null;
10031031
}

src/System.Net.HttpListener/tests/AuthenticationTests.cs renamed to src/System.Net.HttpListener/tests/HttpListenerAuthenticationTests.cs

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
namespace System.Net.Tests
1515
{
16-
public class AuthenticationTests : IDisposable
16+
public class HttpListenerAuthenticationTests : IDisposable
1717
{
1818
private const string Basic = "Basic";
1919
private const string TestUser = "testuser";
@@ -22,14 +22,16 @@ public class AuthenticationTests : IDisposable
2222
private HttpListenerFactory _factory;
2323
private HttpListener _listener;
2424

25-
public AuthenticationTests()
25+
public HttpListenerAuthenticationTests()
2626
{
2727
_factory = new HttpListenerFactory();
2828
_listener = _factory.GetListener();
2929
}
3030

3131
public void Dispose() => _factory.Dispose();
3232

33+
// [ActiveIssue(20840, TestPlatforms.Unix)] // Managed implementation connects successfully.
34+
// [ActiveIssue(17462, TargetFrameworkMonikers.Uap)]
3335
[ConditionalTheory(nameof(Helpers) + "." + nameof(Helpers.IsWindowsImplementationAndNotUap))] // Managed implementation connects successfully.
3436
[InlineData("Basic")]
3537
[InlineData("NTLM")]
@@ -46,6 +48,24 @@ public async Task NoAuthentication_AuthenticationProvided_ReturnsForbiddenStatus
4648
}
4749
}
4850

51+
// [ActiveIssue(20840, TestPlatforms.Unix)] // Managed implementation connects successfully.
52+
// [ActiveIssue(17462, TargetFrameworkMonikers.Uap)]
53+
[ConditionalTheory(nameof(Helpers) + "." + nameof(Helpers.IsWindowsImplementationAndNotUap))] // Managed implementation connects successfully.
54+
[InlineData("Basic")]
55+
[InlineData("NTLM")]
56+
[InlineData("Negotiate")]
57+
[InlineData("Unknown")]
58+
public async Task NoAuthenticationGetContextAsync_AuthenticationProvided_ReturnsForbiddenStatusCode(string headerType)
59+
{
60+
_listener.AuthenticationSchemes = AuthenticationSchemes.None;
61+
62+
using (HttpClient client = new HttpClient())
63+
{
64+
client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(headerType, "body");
65+
await AuthenticationFailureAsyncContext(client, HttpStatusCode.Forbidden);
66+
}
67+
}
68+
4969
[ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotOneCoreUAP))]
5070
[InlineData(AuthenticationSchemes.Basic)]
5171
[InlineData(AuthenticationSchemes.Basic | AuthenticationSchemes.None)]
@@ -390,7 +410,6 @@ public async Task<HttpResponseMessage> AuthenticationFailure(HttpClient client,
390410
var tokenSource = new CancellationTokenSource();
391411
Task<HttpListenerContext> serverTask = Task.Run(() => _listener.GetContext(), tokenSource.Token);
392412

393-
// The client task should complete first - the server should send a 401 response.
394413
Task resultTask = await Task.WhenAny(clientTask, serverTask);
395414
tokenSource.Cancel();
396415
if (resultTask == serverTask)
@@ -404,6 +423,23 @@ public async Task<HttpResponseMessage> AuthenticationFailure(HttpClient client,
404423
return clientTask.Result;
405424
}
406425

426+
public async Task<HttpResponseMessage> AuthenticationFailureAsyncContext(HttpClient client, HttpStatusCode errorCode)
427+
{
428+
Task<HttpResponseMessage> clientTask = client.GetAsync(_factory.ListeningUrl);
429+
Task<HttpListenerContext> serverTask = _listener.GetContextAsync();
430+
431+
Task resultTask = await Task.WhenAny(clientTask, serverTask);
432+
if (resultTask == serverTask)
433+
{
434+
await serverTask;
435+
}
436+
437+
Assert.Same(clientTask, resultTask);
438+
439+
Assert.Equal(errorCode, clientTask.Result.StatusCode);
440+
return clientTask.Result;
441+
}
442+
407443
private async Task ValidateNullUser()
408444
{
409445
Task<HttpListenerContext> serverContextTask = _listener.GetContextAsync();

‎src/System.Net.HttpListener/tests/System.Net.HttpListener.Tests.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='netstandard-Debug|AnyCPU'" />
1010
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='netstandard-Release|AnyCPU'" />
1111
<ItemGroup>
12-
<Compile Include="AuthenticationTests.cs" />
1312
<Compile Include="GetContextHelper.cs" />
13+
<Compile Include="HttpListenerAuthenticationTests.cs" />
1414
<Compile Include="HttpListenerContextTests.cs" />
1515
<Compile Include="HttpListenerPrefixCollectionTests.cs" />
1616
<Compile Include="HttpListenerResponseTests.Cookies.cs" />

0 commit comments

Comments
 (0)