fix pinning in quic by wfurt · Pull Request #52368 · dotnet/runtime

wfurt · 2021-05-06T08:03:09Z

This should fix the native crashes and corrupted buffers. It would be great if you can give it try @CarnaViire

To avoid constant pinning of SendQuicBuffers, I decided to allocate global memory since that is only passed to native msquic. I also added Pending state to indicate that we are actively writing and waiting for msquic callback.

I stills see some timeouts on my 2 core VM.
I will submit separate PR to make some adjustments and to avoid hardcoded values. (e.g. use PassingTestTimeout)

contributes to #52047

ghost · 2021-05-06T08:03:15Z

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

Issue Details

This should fix the native crashes and corrupted buffers. It would be great if you can give it try @CarnaViire

To avoid constant pinning of SendQuicBuffers, I decided to allocate global memory since that is only passed to native msquic. I also added Pending state to indicate that we are actively writing and waiting for msquic callback.

I stills see some timeouts on my 2 core VM.
I will submit separate PR to make some adjustments and to avoid hardcoded values. (e.g. use PassingTestTimeout)

contributes to #52047

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

davidfowl · 2021-05-06T08:25:38Z

+        public SafeMsQuicBufferHandle(int count)
+            : base(IntPtr.Zero, ownsHandle: true)
+        {
+            IntPtr buffer = Marshal.AllocHGlobal(sizeof(QuicBuffer) * count);


We shouldn't need to do this? What pattern is causing this to be required?

Do you mean AllocHGlobal or wrapping it in SafeMsQuicBufferHandle?

Both. We do a similar thing in the ASP.NET Core servers and in sockets and we don't use this pattern AFAIK. What makes MSQuic unique here?

I agree that the SafeHandle should be unnecessary here. The code has to be careful about managing the lifetimes of the async buffers. SafeHandle does not make it easier.

It is an interesting question whether it is better to use unmanaged memory or POH-allocated memory here.

Here is another example from SslStream.

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

Lines 262 to 268 in 7ea26e6

const int NumSecBuffers = 4; // header + data + trailer + empty

Interop.SspiCli.SecBuffer* unmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[NumSecBuffers];

Interop.SspiCli.SecBufferDesc sdcInOut = new Interop.SspiCli.SecBufferDesc(NumSecBuffers)

{

pBuffers = unmanagedBuffer

};

fixed (byte* outputPtr = output)

Because we need to only live through the one native call, we can allocate the buffer array on stack. For msquic we need to wait until HandleEventSendComplete() is invoked. So we need to keep it alive past the sending function.

Anyway, we could use the pinning if we want to but as I mentioned this looked simpler and less work on each call.

For example you have to be careful about situation when somebody disposes the object while the operation is still in flight.

Yep, I'm aware and we also go back and forth on how useful SafeHandles are in certain places, but I'm surprised SafeHandle + native buffer is the solution here. Are we pooling these things?

there is no pool at the moment. The main goal was to stabilize the test runs and avoid corruption and crashes and to unblock @ManickaP and @CarnaViire. I did 700+ runs and I did not see any failure. I think it would be better to refactor and/or optimize once we have solid CI in place.

I think we will need more work toward #5262 and #32142. The SafeHandle primarily helps with p/invokes. For writes, we will need to preserve the memory longer unless we change the handout model.

SafeHandle does not help you much here. You are directly managing the lifetime of the MemoryHandles for the individual buffer, so you can directly managed the lifetime for the array of buffer pointers as well. SafeHandle here is a lot of overhead for negligible benefit.

I understand @jkotas. When we agree on #5262 and #32142 I will either (1) use native memory directly or (2) fall back to pinning on each call. I will also add test for concurrent dispose(s) and IO. I'm pretty sure we still have gaps there. (and possibly investigate and fix #52048 as well)

antonfirsov · 2021-05-06T12:08:47Z

Why not to use Pinned Object Heap instead of native allocation & safe handles? (GC.AllocateUninitializedArray<QuicBuffer>(1, pinned:true))

CarnaViire · 2021-05-06T12:29:11Z

It would be great if you can give it try @CarnaViire

I've tried the current fix and it worked for me -- I haven't seen neither AVE nor buffer corruptions on multiple runs. System.Net.Quic.Tests.MsQuicTests.WriteTests also seems to be fixed by that, so ActiveIssue can be removed from it too.

jkotas · 2021-05-06T15:06:25Z

Why not to use Pinned Object Heap instead of native allocation & safe handles?

One still has to be very careful about lifetime management and ensuring that the pinned buffer is not collected while the unmanaged callback still works on it. Using POH won't make the code simpler.

Also, POH has a problematic performance characteristics today. It always allocates in Gen2 and thus using POH will unnecessarily put pressure on the expensive Gen2 GC collections.

scalablecory

Lets get this merged to unblock other work. We have yet to do a full optimization pass, and I expect we will revisit this (likely removing the new safe handle type) at that point.

jkotas · 2021-05-12T03:49:32Z

Lets get this merged to unblock other work.

I do not see the point merging this with the useless safehandle handle. It is like 10 minute edit to delete it.

wfurt · 2021-05-12T03:58:20Z

Lets get this merged to unblock other work.

I do not see the point merging this with the useless safehandle handle. It is like 10 minute edit to delete it.

and keep the native memory or go back to pinning?

jkotas · 2021-05-12T04:00:11Z

Keep the native memory.

…ons/MsQuic/MsQuicStream.cs Co-authored-by: Cory Nelson <phrosty@gmail.com>

wfurt · 2021-05-12T05:52:26Z

I removed the SafeMsQuicBufferHandle and did ~ 30 test runs on whole QUIC test suite. I will let the tests run in loop overnight.

jkotas · 2021-05-12T06:24:33Z

-
-            // Handle to pinned SendQuicBuffers.
-            public GCHandle SendHandle;
+            public IntPtr SendQuicBuffers  = Marshal.AllocHGlobal(sizeof(QuicBuffer));


It would be better to allocate this only once it is needed, with the right size.

As written, SendQuicBuffers maybe be allocated and never used; or SendQuicBuffers may leak if the initialization fails mid-flight.

ok. I can move it to constructor at some point or add logic to SendReadOnlyMemoryAsync(). The old code always allocated array of size 1 to optimize for using ReadOnlyMemory e.g. use from HttpClient.

jkotas

LGTM otherwise

ManickaP

Just questions to understand the change better. Otherwise, LGTM.

ManickaP · 2021-05-12T07:37:18Z

        private static void CleanupSendState(State state)
        {
-            if (state.SendHandle.IsAllocated)
+            lock (state)


Why the lock here?

We lock the state in other place when we transition. So I think we should be either consists or avoid the lock completely via some other mechanism (like preventing duplicate operations)

ManickaP · 2021-05-12T07:47:27Z

        {
+            lock (_state)
+            {
+                Debug.Assert(_state.SendState != SendState.Pending);


I guess we want to prevent overlapping Sends. Shouldn't this then rather be a condition with throw QuicException...?

This was not meant as guard agains overlaying Sends. (I think that should be done much earlier)
I added this to make sure our internal state logic (and msquic) always moves to some other state.

ManickaP · 2021-05-12T08:14:03Z

-            _state.BufferArrays[0] = handle;
+            if (_state.SendQuicBuffers == IntPtr.Zero)
+            {
+                _state.SendQuicBuffers = Marshal.AllocHGlobal(sizeof(QuicBuffer));


So the major difference here is that we allocate SendQuicBuffers aka QuicBuffer* directly in unmanaged memory. While originally we were using pinned managed memory GCHandle.Alloc(_state.SendQuicBuffers, GCHandleType.Pinned);.

What was wrong with the original approach? Was it Marshal.UnsafeAddrOfPinnedArrayElement(_state.SendQuicBuffers, 0);? Should that be _state.SendHandle.AddrOfPinnedObject();?

This comment is just me trying to properly understand what's going on here. It has absolutely no influence on mergeability of this PR 😄

Yes, we could keep pinning. But then we do more operations on each send and we need to maintain the handle. So I felt this would be simpler as the SendQuicBuffers are really only consumed by native code.

The general point here is that since we only use this array as unmanaged memory, there's not really any benefit to allocating it as managed memory, and some nontrivial cost (overhead of pin/unpin, additional GC overhead, etc).

But how does this solve the problem with byte mixing and AVE? Do we know the exact root cause of that?

I have doubts that this change is actually fixing the root cause of the crash. It is probably just moving it around.

This type has number of subtle issues like #52048 that will lead to use-after-free and similar crashes. Somebody will need to do a focused pass over it to fix them.

But how does this solve the problem with byte mixing and AVE? Do we know the exact root cause of that?

Yeah, you are right. Seems like this probably doesn't fix the root cause. It might make it less likely to happen, because the memory will never move; but I don't think we actually understand the root cause yet.

(I had assumed we were previously allocating the array on the stack, per discussion above -- but we weren't, so that wasn't the root problem.)

It would probably help to clear out the BufferArray entries when we dispose them, here: https://github.com/dotnet/runtime/pull/52368/files#diff-55ed6a1c110b1a90d4900bce3075ab49f1d6212c223e69b48f711f4084d264acR787

If we have a use-after-free issue, that would help find it.

It seems suspicious that we are calling CleanupSendState in Dispose -- how do we know that msquic isn't still holding our buffer array?

@jkotas

This type has number of subtle issues like #52048 that will lead to use-after-free and similar crashes. Somebody will need to do a focused pass over it to fix them.

Yeah, I agree we need a pass here. Do you have specific concerns? Dispose in particular looks questionable to me, anything else?

anything else?

Handling of different failure modes. For example:

https://github.com/dotnet/runtime/pull/52368/files#diff-55ed6a1c110b1a90d4900bce3075ab49f1d6212c223e69b48f711f4084d264acR943-R945

If AllocHGlobal fails, we will think that we have space for old SendBufferMaxCount, but SendQuicBuffers is actually going to be null. This will lead to NullReferenceException on retry.
If new MemoryHandle[array.Length] fails, we will think that we have space for new SendBufferMaxCount, but BufferArrays is actually going to have only have space for old SendBufferMaxCount. This will lead to IndexOutOfBoundsException on retry.

I'm going to write new tests and cleanup the shutdown more. I would generally expect that if allocation fails, the stream (or whole system) will not be in usable state. I can certainly split the allocations and assignment to make sure it is consistent.

fix pinning in quic

9241457

wfurt requested review from CarnaViire, ManickaP, geoffkizer and scalablecory May 6, 2021 08:03

ghost added the area-System.Net label May 6, 2021

davidfowl reviewed May 6, 2021

View reviewed changes

scalablecory approved these changes May 12, 2021

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated

Comment thread ...System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicBufferHandle.cs Outdated

wfurt and others added 2 commits May 11, 2021 22:45

remove SafeMsQuicBufferHandle

087fad4

Update src/libraries/System.Net.Quic/src/System/Net/Quic/Implementati…

7a4fb7d

…ons/MsQuic/MsQuicStream.cs Co-authored-by: Cory Nelson <phrosty@gmail.com>

jkotas reviewed May 12, 2021

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

jkotas reviewed May 12, 2021

View reviewed changes

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

Apply suggestions from code review

e5c9877

jkotas reviewed May 12, 2021

View reviewed changes

jkotas approved these changes May 12, 2021

View reviewed changes

more feedback from review

86dd8cd

ManickaP approved these changes May 12, 2021

View reviewed changes

wfurt merged commit 8eb7692 into dotnet:main May 12, 2021

wfurt deleted the quic_52047 branch May 12, 2021 20:06

This was referenced May 13, 2021

[QUIC] Byte mixing and/or native access violation failures #52047

Closed

[QUIC] Managed Access Violation in MsQuicStream finalizer #52048

Closed

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

	const int NumSecBuffers = 4; // header + data + trailer + empty
	Interop.SspiCli.SecBuffer* unmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[NumSecBuffers];
	Interop.SspiCli.SecBufferDesc sdcInOut = new Interop.SspiCli.SecBufferDesc(NumSecBuffers)
	{
	pBuffers = unmanagedBuffer
	};
	fixed (byte* outputPtr = output)

Conversation

wfurt commented May 6, 2021

Uh oh!

ghost commented May 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarnaViire commented May 6, 2021

Uh oh!

jkotas commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkotas commented May 12, 2021

Uh oh!

wfurt commented May 12, 2021

Uh oh!

jkotas commented May 12, 2021

Uh oh!

wfurt commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl May 6, 2021 •

edited

Loading

antonfirsov commented May 6, 2021 •

edited

Loading

jkotas commented May 6, 2021 •

edited

Loading

jkotas May 13, 2021 •

edited

Loading