[QUIC] Managed Access Violation in MsQuicStream finalizer

[CarnaViire]

Repro:

        [Fact]
        public async Task ManagedAVE_MinimalFailingTest()
        {
            async Task GetStreamIdWithoutStartWorks()
            {
                using QuicListener listener = CreateQuicListener();
                using QuicConnection clientConnection = CreateQuicConnection(listener.ListenEndPoint);

                ValueTask clientTask = clientConnection.ConnectAsync();
                using QuicConnection serverConnection = await listener.AcceptConnectionAsync();
                await clientTask;

                using QuicStream clientStream = clientConnection.OpenBidirectionalStream();
                Assert.Equal(0, clientStream.StreamId);

                // TODO: stream that is opened by client but left unaccepted by server may cause AccessViolationException in its Finalizer
            }

            await GetStreamIdWithoutStartWorks();

            GC.Collect();
        }

For me it now fails on every run, with a stacktrace

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Net.Quic.Implementations.MsQuic.Internal.SafeMsQuicStreamHandle.ReleaseHandle()
   at System.Runtime.InteropServices.SafeHandle.InternalRelease(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Dispose(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Dispose()
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.Dispose(Boolean)
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.Finalize()

As mentioned before in TODO comment, the stream that is being finalized is the one left inside connection's Channel<MsQuicStream> AcceptQueue - it was created and put there on PEER_STREAM_STARTED event, but was never claimed by calling AcceptStreamAsync. I didn't find an easy way to dispose what's inside AcceptQueue on connection's Dispose. But maybe that's not the only problem - users might easily leave QuicStreams undisposed, but it shouldn't lead to AVE.

[ghost]

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

Issue Details

---

Repro:

        [Fact]
        public async Task ManagedAVE_MinimalFailingTest()
        {
            async Task GetStreamIdWithoutStartWorks()
            {
                using QuicListener listener = CreateQuicListener();
                using QuicConnection clientConnection = CreateQuicConnection(listener.ListenEndPoint);

                ValueTask clientTask = clientConnection.ConnectAsync();
                using QuicConnection serverConnection = await listener.AcceptConnectionAsync();
                await clientTask;

                using QuicStream clientStream = clientConnection.OpenBidirectionalStream();
                Assert.Equal(0, clientStream.StreamId);

                // TODO: stream that is opened by client but left unaccepted by server may cause AccessViolationException in its Finalizer
            }

            await GetStreamIdWithoutStartWorks();

            GC.Collect();
        }

For me it now fails on every run, with a stacktrace

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Net.Quic.Implementations.MsQuic.Internal.SafeMsQuicStreamHandle.ReleaseHandle()
   at System.Runtime.InteropServices.SafeHandle.InternalRelease(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Dispose(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Dispose()
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.Dispose(Boolean)
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.Finalize()

As mentioned before in TODO comment, the stream that is being finalized is the one left inside connection's Channel<MsQuicStream> AcceptQueue - it was created and put there on PEER_STREAM_STARTED event, but was never claimed by calling AcceptStreamAsync. I didn't find an easy way to dispose what's inside AcceptQueue on connection's Dispose. But maybe that's not the only problem - users might easily leave QuicStreams undisposed, but it shouldn't lead to AVE.

Author:	CarnaViire
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	-

[geoffkizer]

I didn't find an easy way to dispose what's inside AcceptQueue on connection's Dispose.

Seems like we should drain the AcceptQueue on connection Dispose and Dispose those streams as well.

[CarnaViire]

Seems like we should drain the AcceptQueue on connection Dispose and Dispose those streams as well.

This would fix the symptom, but I believe the same exception might also happen if the stream was accepted but then was forgotten and left undisposed for the finalizer. So we may want to search for the actual root cause.

[ManickaP]

I haven't tested @wfurt fix yet, but I got the AVE twice in row now and according to the dumps the buffers are messed up:

(lldb) f
frame #1: 0x00007f09739a8dfd libmsquic.so`QuicStreamSendBufferRequest(Stream=0x00007f08a403ab00, Req=0x00007f08bc0182f0) at stream_send.c:445:13
   442          }
   443          uint8_t* CurBuf = Buf;
   444          for (uint32_t i = 0; i < Req->BufferCount; i++) {
-> 445              CxPlatCopyMemory(
   446                  CurBuf, Req->Buffers[i].Buffer, Req->Buffers[i].Length);
   447              CurBuf += Req->Buffers[i].Length;
   448          }
(lldb) p *Req
(QUIC_SEND_REQUEST) $1 = {
  Next = 0x0000000000000000
  Buffers = 0x00007f098e7f9cc8
  BufferCount = 1
  Flags = QUIC_SEND_FLAG_NONE
  StreamOffset = 0
  TotalLength = 50
  InternalBuffer = (Length = 1414029138, Buffer = "")
  ClientContext = 0x0000000000000000
}
(lldb) p Req->Buffers
(const QUIC_BUFFER *) $2 = 0x00007f098e7f9cc8
(lldb) p *(Req->Buffers)
!!!(const QUIC_BUFFER) $3 = (Length = 2390727928, Buffer = 0x0000000000000000)!!!
(lldb) 

I.e. nothing new, just confirming the problem with some more detail from the dump.

[wfurt]

I can take a look but I was not sure if you are already into it @ManickaP. There was useful feedback in #52368 so I'll continue to improve object management.

[ManickaP]

I'm not digging into it. I'm just confirming the problem and sharing some details (they might be useful). But if you want me to dig deeper into the dump or test some fixes locally, I can do that. I'm weird and I like to play with lldb 😄.