Fix DNS cancellation deadlock #63904

antonfirsov · 2022-01-17T22:41:39Z

As shown by #63552 (comment), in case the completion triggers before cancellation, GetAddrInfoExCancel does not return while the completion routine is running. This results in a deadlock if we are taking the same lock in completion and cancellation callbacks.

It looks to me that the lock is unnecessary, since GetAddrInfoExContext* is the only shared state, and pointer access is atomic.

As a prerequisite, I had to address #43816, so we can have reliable cancellation tests. I did this by serializing the tests, which eliminates parallel load as an instability factor causing races between CPU and IO-bound code.

Fixes #63552.
Resolves #43816. (If not, we can reopen.)

ghost · 2022-01-17T22:41:47Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

As shown by #63552 (comment), in case the completion triggers before cancellation, GetAddrInfoExCancel does not return while the completion routine is running. This results in a deadlock if we are taking the same lock in completion and cancellation callbacks.

It looks to me that the lock is unnecessary, since GetAddrInfoExContext* is the only shared state, and pointer access is atomic.

As a prerequisite, I had to address #43816, so we can have reliable cancellation tests. I did this by serializing the tests, which eliminates parallel load as an instability factor causing races between CPU and IO-bound code.

Fixes #63552.
Resolves #43816. (If not, we can reopen.)

Author:	antonfirsov
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

ManickaP

LGTM, thanks.

antonfirsov · 2022-01-18T17:38:07Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-01-18T17:38:22Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

scalablecory · 2022-01-24T22:29:23Z

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

@@ -409,17 +411,14 @@ public void RegisterForCancellation(CancellationToken cancellationToken)
                        var @this = (GetAddrInfoExState)o!;
                        int cancelResult = 0;

-                        lock (@this)


Is there a use-after-free race condition here with context being freed at the end of ProcessResult?

Good catch, I overlooked this race. Looks like we should implement a more intrusive change adding some sort of ref-counting.

antonfirsov · 2022-01-31T17:51:41Z

@scalablecory @geoffkizer any idea why do we prefer unmanaged AllocHGlobal allocation instead of embedding GetAddrInfoExContext into GetAddrInfoExState and pinning the managed object?

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

Line 503 in 57bfe47

    
           var context = (GetAddrInfoExContext*)Marshal.AllocHGlobal(sizeof(GetAddrInfoExContext));

We allocate a GCHandle for GetAddrInfoExState anyways:

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

Line 482 in 57bfe47

    
           public IntPtr CreateHandle() => GCHandle.ToIntPtr(GCHandle.Alloc(this, GCHandleType.Normal));

It seems to me that the straightforward way to address #63904 (comment) is to introduce some reference counting. If it's reasonable to embed GetAddrInfoExContext into GetAddrInfoExState, the SafeHandle model doesn't fit this case. Is there an established way to implement custom ref counting in libraries? I see DeferredDisposableLifetime, but it's an internal of System.Private.CoreLib.

geoffkizer · 2022-01-31T21:26:48Z

We allocate a GCHandle for GetAddrInfoExState anyways:

Yeah, but the GCHandle here is not pinned. Maybe it could be, but I'm not sure of the implications of that.

antonfirsov · 2022-01-31T21:41:11Z

I realized that the GC cannot pin GetAddrInfoExState since it contains managed references. Addressed the use-after-free race condition by turning GetAddrInfoExState int a SafeHandle guarding GetAddrInfoExContext*. (a2a9355)

scalablecory · 2022-02-01T09:48:37Z

@scalablecory @geoffkizer any idea why do we prefer unmanaged AllocHGlobal allocation instead of embedding GetAddrInfoExContext into GetAddrInfoExState and pinning the managed object?

Not sure why we use that instead of just pinning an array from the array pool. Hard to know how perf would change.

jkotas · 2022-02-01T12:04:41Z

Not sure why we use that instead of just pinning an array from the array pool.

If you need a piece of pinned unmanaged memory, it is easier to allocate it using AllocHGlobal. The performance difference is negligible, also depends on what you measure. Pinning has negative impact on the overall GC performance that is hard to measure using microbenchmarks.

You can switch from AllocHGlobal to NativeMemory.Alloc/Free while you are on it. NativeMemory.Alloc/Free looks better and it is also a bit faster than AllocHGlobal.

karelz · 2022-02-08T17:10:58Z

@ManickaP can you please re-review latest changes? Thanks!

CarnaViire · 2022-02-08T17:29:17Z

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

@@ -194,10 +194,10 @@ private static unsafe void GetAddressInfoExCallback(int error, int bytes, Native

        private static unsafe void ProcessResult(SocketError errorCode, GetAddrInfoExContext* context)
        {
+            GetAddrInfoExState state = GetAddrInfoExState.FromHandleAndFree(context->QueryStateHandle);


Why it is not inside try anymore?

Because state is being disposed now in the finally block, so it has to live out the try scope. FromHandleAndFree should only throw if there is a bug in our code.

The state can be just declared outside the try-finally and conditionally disposed, the same way as it's in GetAddrInfoAsync.
But I don't mind either way. If this throws, we have a bigger problem than missing Dispose call.

That won't help unfortunately. If the FromHandleAndFree throws, no value will be assigned to state, so there will be nothing to dispose.

ManickaP

LGTM, thanks!

ManickaP · 2022-02-10T12:04:17Z

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

@@ -194,10 +194,10 @@ private static unsafe void GetAddressInfoExCallback(int error, int bytes, Native

        private static unsafe void ProcessResult(SocketError errorCode, GetAddrInfoExContext* context)
        {
+            GetAddrInfoExState state = GetAddrInfoExState.FromHandleAndFree(context->QueryStateHandle);


The state can be just declared outside the try-finally and conditionally disposed, the same way as it's in GetAddrInfoAsync.
But I don't mind either way. If this throws, we have a bigger problem than missing Dispose call.

antonfirsov · 2022-02-22T21:37:14Z

OuterLoop failures are #65648.

ManickaP · 2022-03-08T16:17:02Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-03-08T16:17:17Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2022-03-09T00:59:26Z

The re-enabled test, DnsGetHostEntry_PostCancelledToken_Throws failed on Windows 7, which might be the result of flakiness experienced the weird win7 test environment. We may just disable the test on that OS.

Rerunning for now.

antonfirsov · 2022-03-09T00:59:36Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-03-09T00:59:54Z

Azure Pipelines successfully started running 1 pipeline(s).

deeprobin · 2022-03-09T19:38:06Z

src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs

+
+        // This is a regression test for https://github.com/dotnet/runtime/issues/63552
+        [Fact]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/33378", TestPlatforms.AnyUnix)] // Cancellation of an outstanding getaddrinfo is not supported on *nix.


As described in #63902 we try to reduce the amount of workarounds and test-ignores if the regarding issue is closed.
#33378 is closed. So imo this can be removed

Suggested change

[ActiveIssue("https://github.com/dotnet/runtime/issues/33378", TestPlatforms.AnyUnix)] // Cancellation of an outstanding getaddrinfo is not supported on *nix.

The PR #34633 that closed that issue did not add cancellation support and it has been reverted in #48666 in the end.

As a result [ActiveIssue] points to un outdated and closed issue and we are not tracking this problem properly today. This is something we should clean up, but I would prefer to address this later, and not to block this PR even longer.

antonfirsov · 2022-03-21T18:38:55Z

Test failures are unrelated, mostly #66803.

MartyIX · 2022-03-22T06:49:03Z

Thank you for the bug fix. Happy to re-test once it's released. 🙏

rzikm · 2022-03-29T14:04:07Z

/backport to release/6.0

github-actions · 2022-03-29T14:07:22Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2058994928

Avoid taking a lock, and address the use-after-free race condition by guarding GetAddrInfoExContext with a SafeHandle.

antonfirsov added 5 commits January 17, 2022 21:57

tests

ccaa17e

remove lock

4e34167

comments, outerloop

3d5043e

use Volatile.Write

a8b262f

test comments

8b9c79b

dotnet-issue-labeler bot added the area-System.Net label Jan 17, 2022

ghost assigned antonfirsov Jan 17, 2022

This comment has been minimized.

Sign in to view

antonfirsov requested a review from a team January 17, 2022 22:42

This comment has been minimized.

Sign in to view

ManickaP approved these changes Jan 18, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

scalablecory reviewed Jan 24, 2022

View reviewed changes

karelz added this to the 7.0.0 milestone Jan 25, 2022

SafeHandle to avoid use-after-free race condition

a2a9355

use NativeMemory APIs

81234a4

runfoapp bot mentioned this pull request Feb 8, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

CarnaViire reviewed Feb 8, 2022

View reviewed changes

ManickaP approved these changes Feb 10, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment was marked as duplicate.

Sign in to view

dotnet deleted a comment from azure-pipelines bot Feb 22, 2022

deeprobin suggested changes Mar 9, 2022

View reviewed changes

Disable DnsGetHostEntry_PostCancelledToken_Throws on Win7

7d93bd7

This comment was marked as resolved.

Sign in to view

runfoapp bot mentioned this pull request Mar 21, 2022

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

antonfirsov merged commit 548b70d into dotnet:main Mar 21, 2022

github-actions bot mentioned this pull request Mar 29, 2022

[release/6.0] Fix DNS cancellation deadlock #67291

Merged

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022

Fix DNS cancellation deadlock (dotnet#63904)

a1ad056

Avoid taking a lock, and address the use-after-free race condition by guarding GetAddrInfoExContext with a SafeHandle.

karelz mentioned this pull request Apr 19, 2022

Dns.GetHostAddressesAsync gets stuck on Windows #63552

Closed

ghost locked as resolved and limited conversation to collaborators Apr 28, 2022

Fix DNS cancellation deadlock #63904

Fix DNS cancellation deadlock #63904

Conversation

antonfirsov commented Jan 17, 2022

ghost commented Jan 17, 2022

This comment has been minimized.

This comment has been minimized.

ManickaP left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

antonfirsov commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

Choose a reason for hiding this comment

antonfirsov Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Jan 31, 2022 • edited Loading

geoffkizer commented Jan 31, 2022

antonfirsov commented Jan 31, 2022 • edited Loading

scalablecory commented Feb 1, 2022

jkotas commented Feb 1, 2022

karelz commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as duplicate.

antonfirsov commented Feb 22, 2022

ManickaP commented Mar 8, 2022

azure-pipelines bot commented Mar 8, 2022

antonfirsov commented Mar 9, 2022

antonfirsov commented Mar 9, 2022

azure-pipelines bot commented Mar 9, 2022

Choose a reason for hiding this comment

antonfirsov Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

antonfirsov commented Mar 21, 2022

MartyIX commented Mar 22, 2022

rzikm commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

antonfirsov Jan 31, 2022 •

edited

Loading

antonfirsov commented Jan 31, 2022 •

edited

Loading

antonfirsov commented Jan 31, 2022 •

edited

Loading

antonfirsov Mar 21, 2022 •

edited

Loading